Hi Yosry, On Thu, Jul 11, 2019 at 4:10 AM Yosry Muhammad <yosry...@gmail.com> wrote:
> Hi, > Please find an updated patch attached. > > On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi < > khushboo.va...@enterprisedb.com> wrote: > >> Hi Yosry, >> >> I liked the way you have refactored the code at some places in the JS >> file and made it cleaner. >> Here are some points: >> >> 1. The table (including partition table) with a single column having that >> column primary key is editable but the save button is disabled, so, >> ultimately I can't save the data. Note: The table should be empty to >> reproduce this issue. >> > > I tried to reproduce the issue but it works fine for me. Please make sure > that you edit the empty cell (to add a new row) and press enter for the > edits on the grid to take effect. > > Right, on the enter, the button gets enabled, not on the focus out, so this is by design, not something your patch caused. > 2. command.py - The check_updatable_results_pkeys function calling the >> poll function and checks the ASYNC_OK, I think this is not required as this >> function is called from the poll function from the sqleditor/__init__.py >> *if the status of the polling is if ASYNC_OK*. So, I think this is overhead >> but if you have considered another scenario then let me know. >> > > It was just a sanity check, just in case someone calls the function > incorrectly. I removed it and added comments below the function header > indicating when it should be called. > Please make sure to remove "from pgadmin.tools.sqleditor.utils.constant_definition import ASYNC_OK" line from the file as not required anymore. > > >> 3. In the Preferences, the label of the keyboard shortcut "Save Data >> Changes" should be "Save data changes". >> > > Corrected. > > >> 4. Dave has already mentioned about the commented code, so I do agree we >> should remove it. >> > > Removed. > > >> 5. I didn't find the doc updates for the keyboard shortcuts in the >> Preferences module as well as related to this feature. Am I missing >> something? >> >> > I don't know which part exactly are you referring to. > In the previous patch, I have already updated keyboard_shortcuts.rst to > document the new button shortcut, in addition to preferences.rst to > document the new 'Alert on uncommited changes' option. I also updated > query_tool.rst, query_tool_toolbar.rst & editgrid.rst to document the new > features. Which part is missing? > > My bad, I have applied the patch on the web folder, so the difference of rst files didn't get applied. Now, I can see the doc updates and it looks good to me. Looking forward to your feedback and comments. > Thanks ! > Thanks, Khushboo
