Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hlQos-0001TD-0J for pgadmin-hackers@arkaria.postgresql.org; Thu, 11 Jul 2019 04:36:34 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hlQoq-0005WF-L0 for pgadmin-hackers@arkaria.postgresql.org; Thu, 11 Jul 2019 04:36:32 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hlQoq-0005Vj-9R for pgadmin-hackers@lists.postgresql.org; Thu, 11 Jul 2019 04:36:32 +0000 Received: from mail-ot1-x334.google.com ([2607:f8b0:4864:20::334]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hlQon-0004ao-4n for pgadmin-hackers@postgresql.org; Thu, 11 Jul 2019 04:36:31 +0000 Received: by mail-ot1-x334.google.com with SMTP id r21so4512005otq.6 for ; Wed, 10 Jul 2019 21:36:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=1FXyQvU4PocwhdMa6L48AbiCPuQsCkhlCBdxmm8/5Qk=; b=WwtEDGAB+njXHAIO/gIkUEJEFZLi1tgvA5dJ5G+cP630M4MzeKSc8gsWZ0vizSdGrz OmgREjTeCra8Rv8ytlOiYJ9F6t3/cfgEjj6Zf3GCLQXyTdT8tO/NUkqIpIPIATjois7b ajo9b4se7pk1oaRt9SvPavvk8RTJdDy6YCXMQUzVoRKP+zKcGy/jYqkfrNoAvyic1L9h RtZqrlHOe+XL6N8YoU+P7J5z+7oqIsTx72qz2VV5i3ywFmWcZHu/Y6T64WSAHV/Vr0UY AwoV9kqKlp60exIsXdGx6XHZGteCW2sDGRvvtPKvDoGyYD2H41ll16uD46o8a9tEFq3R YZTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=1FXyQvU4PocwhdMa6L48AbiCPuQsCkhlCBdxmm8/5Qk=; b=BK7HMz0SLi4M6gjmVtDOCYu/VxbE4YWoc9prVOlpJagitg7HbJlkf/EJ944DrKH+J2 NiJNB8C4483+liNJlOWZB26fsrzmwj8JgXEci7J6k4qW4ndnZqdQgLQEOImt1RsjrAsn nxT5Y2L5A/8qsKKfqW6Rcwy1eWfZcdUCALDn6YDl2Onk0HTt/3ZMiBkJtkUDm5LgNzzk WIzv9fTPv/MgIfwofSq/LfxPxb0nuNyoCaZ1nnPEudqgANR3vKqK2T6UKp1JAoB8gxRk wriP6N6X9/8tIFr5rAySb31uvDXpI9fPRFENE5GhKRJFhPNQ3GcOgLxw2P7q9ZN4NSMH nVMg== X-Gm-Message-State: APjAAAVHTRVkojY19LA8qm+nGYcu6qIA6h2HRRwd0qrGnQVXM9KipiFe nm4piua/H9nSlRKypLYRAbbu+/rWl9lua7YSNuuyfA== X-Google-Smtp-Source: APXvYqxxNA8ljCy252jv1iQqkQmfY/DITCSYbHF53zXDP4C/Lqmkw9ak8rJiyvQ+MH73RC057RHmnf/gpSrqO+ni7kg= X-Received: by 2002:a9d:70da:: with SMTP id w26mr1686760otj.270.1562819786699; Wed, 10 Jul 2019 21:36:26 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Khushboo Vashi Date: Thu, 11 Jul 2019 10:06:17 +0530 Message-ID: Subject: Re: [GSoC] Finalized First Patch To: Yosry Muhammad Cc: Dave Page , pgadmin-hackers , Akshay Joshi Content-Type: multipart/alternative; boundary="0000000000003c1088058d6056bc" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000003c1088058d6056bc Content-Type: text/plain; charset="UTF-8" Hi Yosry, On Thu, Jul 11, 2019 at 4:10 AM Yosry Muhammad wrote: > Hi, > Please find an updated patch attached. > > On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi < > khushboo.vashi@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 --0000000000003c1088058d6056bc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Yosry,

On Thu, Jul 11, 2019 at 4:10 AM = Yosry Muhammad <yosrym93@gmail.com= > wrote:
=
Hi,
Please find an updated patc= h attached.

On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi <khushboo.vash= i@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 colum= n having that column primary key is editable but the save button is disable= d, so, ultimately I can't save the data. Note: The table should be empt= y to reproduce this issue.

I tr= ied 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.
=C2=A0
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 pol= l function and checks the ASYNC_OK, I think this is not required as this fu= nction 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 someon= e 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 pga= dmin.tools.sqleditor.utils.constant_definition import ASYNC_OK" line from the file as not required anymor= e.=C2=A0
=C2=A0
3. In the = Preferences, the label of the keyboard shortcut "Save Data Changes&quo= t; should be "Save data changes".
Corrected.
=C2=A0
4. Dave has already mentione= d about the commented code, so I do agree we should remove it.
<= /blockquote>

Removed.
=C2=A0
5. I didn&#= 39;t find the doc updates for the keyboard shortcuts in the Preferences mod= ule as well as related to this feature. Am I missing something?
<= br>

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 uncommit= ed changes' option. I also updated query_tool.rst, query_tool_toolbar.r= st & editgrid.rst to document the new features. Which part is missing?<= br>
=C2=A0
My bad, I have appl= ied 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.
<= div>
Looking forward to your feedback and = comments.
Thanks !

Thanks,
Khushboo=C2=A0
--0000000000003c1088058d6056bc--