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 1hl6Ad-0004TY-WD for pgadmin-hackers@arkaria.postgresql.org; Wed, 10 Jul 2019 06:33:40 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hl6Ac-0002U4-QG for pgadmin-hackers@arkaria.postgresql.org; Wed, 10 Jul 2019 06:33:38 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hl6Ac-0002Kw-5G for pgadmin-hackers@lists.postgresql.org; Wed, 10 Jul 2019 06:33:38 +0000 Received: from mail-ot1-x341.google.com ([2607:f8b0:4864:20::341]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hl6AZ-0007UB-3f for pgadmin-hackers@postgresql.org; Wed, 10 Jul 2019 06:33:36 +0000 Received: by mail-ot1-x341.google.com with SMTP id s20so1050164otp.4 for ; Tue, 09 Jul 2019 23:33:34 -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=m4/eBaXoajU4ATlI6jPwjpTQcO8Ba5gXqLBykIgE618=; b=Ypo3OrkfbnR/WFV9gn+GdMHuLepQbPQYHBfXT01ZU4jc80GCsaeDnjMy0bbDz6Rs4f So41PIpi6IujK6uEeZNDW9DPXFzqPlYAZLtbOlqn0fBdR2M+oWemkh5qasgXXsYKuNXO a2h0Dnix2lt4nwC4lxIIENe0D0fJaBViEd6MLgN++G1Bb3bNhylOmnQb0OWL96v4985t yrbPi9giMqCF6p+5Gy0uDICE5Efrx5fwUj5rLrL3GjmEwB4OZQGjoZOtyCEfTXyrZWb3 a7SaqwZFgJtkdjI2/3k7o2fcg4oVPSnkmZqdE+b83ajyDYw+PuGPAdbklpPDL9UUHxSS /Yfg== 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=m4/eBaXoajU4ATlI6jPwjpTQcO8Ba5gXqLBykIgE618=; b=fXGNO4Te2SB220/ckEWuA00jNL0hm3NIfQCodiNQuvStSNdkv0zcB5RRrJBtdry/sR aWkDrVQ11PkdVwN/iohpjP+Wnn5mu1Zvw1O5vKshx/B+OGFaXvcNCqjFxIBS1cLiL1LN 7ZnHaMM6lfRo2eMwqXZL87k6vqj1U5o+yrzp3Uc51aUnX2TdS0V5fWY5URZq0zLLy68S heczkedO7Zyy8RBM8Q+ntZbv5tXmB8TX9butYSiCv6+OM2YnMaSNntXr252oCU5aZw1Q z22Xznb1LzhIN5XIR9kuY8Zlczhso+Sf7qykdbAqZG7gU4p2ufHA0JNiH5/vTXf4jE0l cEWg== X-Gm-Message-State: APjAAAWA6EDj/MU1lij3Hcj7Gb406LsxODPjhYYxg2nR7bB531aNQc8Q yfVSI9Tj3huOm9/cW/Q/yDRBpEZKycCp8ZiD3W3UNQ== X-Google-Smtp-Source: APXvYqw3B813AuSFEd95U25wztU5nXwS/JyMKGiD0tHXUZ7cwzMTiy5AwgHEL1SBxaWhxJl7bwvTNQFfpeH0mEJCN8o= X-Received: by 2002:a9d:744a:: with SMTP id p10mr11515otk.321.1562740414178; Tue, 09 Jul 2019 23:33:34 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Khushboo Vashi Date: Wed, 10 Jul 2019 12:03:24 +0530 Message-ID: Subject: Re: [GSoC] Finalized First Patch To: Dave Page Cc: Yosry Muhammad , pgadmin-hackers , Akshay Joshi Content-Type: multipart/alternative; boundary="000000000000437974058d4ddb98" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000437974058d4ddb98 Content-Type: text/plain; charset="UTF-8" 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. 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. 3. In the Preferences, the label of the keyboard shortcut "Save Data Changes" should be "Save data changes". 4. Dave has already mentioned about the commented code, so I do agree we should remove it. 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? Keep doing wonderful work for pgAdmin. :) Thanks, Khushboo On Fri, Jul 5, 2019 at 4:31 PM Dave Page wrote: > Hi > > On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad wrote: > >> - The patch won't apply with "git apply" and only partially applies with >>> patch -p0. Please "git add" all your changes and new files in your repo, >>> and then run "git diff --cached --binary", which should create a usable >>> patch. You can then un-stage your changes. >>> >>> >> That was due to new image. I made an applicable one with the below >> modifications, please find it attached. >> > > Thanks! > > >> >> >>> - execute_query_utils.py is somewhat lacking in file header and any >>> comments. >>> >>> >> Added. >> >> - There is commented-out code in sqleditor.js >>> >> >> This is the call that adds queries that are generated by pgAdmin to the >> query history. I commented it instead of removing it as I will add it later >> with some modifications when I add the checkbox. >> > > Sure, but we won't commit commented-out code. It just makes things messy > until such time as it gets used (or more often, does not). > > >> - On reflection, I don't think the "Data saved successfully, you still >>> need to commit changes to the database." is prominent enough - in testing, >>> I found it very easy to miss. That might also be compounded by the fact >>> that "Alert on uncommitted transactions?" doesn't seem to be working for >>> me. I get the "Save text" prompt to save the query text, but if I say no to >>> that, the Query Tool instance closes with no further warning, despite >>> having a transaction in progress. >>> >> >> I made a separate notification for the uncommitted save to make it more >> visible, check it out. >> > > That's definitely clearer now. It avoids the "blindness" caused by the > fact that you always get the green message. > > >> I tried the scenario you provided and the uncommitted alert worked fine, >> could you please try again and tell me the exact scenario where that >> happened? >> > > Hmm, it's working fine for me today too. Definitely wasn't yesterday > though! > > I'm going to make some minor tweaks to the wording of the docs before I > commit (as well as removing the commented-out code), but I think this is > good to go, once it's had another review. Khushboo, please take a look as > soon as you can. > > Thanks! > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --000000000000437974058d4ddb98 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Yosry,

I like= d the way you have refactored the code at some places in the JS file and ma= de it cleaner.
Here are some points:

1. = The table (including partition table) with a single column having that colu= mn 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 thi= s issue.
2. command.py - The check_updatable_results_pkeys functi= on calling the poll function and checks the ASYNC_OK, I think this is not r= equired as this function is called from the poll function from the sqledito= r/__init__.py *if the status of the polling is if ASYNC_OK*. So, I think th= is is overhead but if you have considered another scenario then let me know= .
3. In the Preferences, the label of the keyboard shortcut "= ;Save Data Changes" should be "Save data changes".
4. Dave has already mentioned about the commented code, so I do agree we s= hould remove it.
5. I didn't find the doc updates for the key= board shortcuts in the Preferences module as well as related to this featur= e. Am I missing something?

Keep doing wonderful wo= rk for pgAdmin. :)

Thanks,
Khushboo
=C2=A0
On Fri, Jul 5, 2019 at 4:31 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <yosrym93@gmail.com> wrote:
- The patch won't apply with "git apply" and on= ly partially applies with patch -p0. Please "git add" all your ch= anges and new files in your repo, and then run "git diff --cached --bi= nary", which should create a usable patch. You can then un-stage your = changes.


That was due t= o new image. I made an applicable one with the below modifications, please = find it attached.

Tha= nks!
=C2=A0
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
- ex= ecute_query_utils.py is somewhat lacking in file header and any comments.

Added.

- There is commented-out code in sqleditor.js
=

This is the call that adds queries that are generated b= y pgAdmin to the query history. I commented it instead of removing it as I = will add it later with some modifications when I add the checkbox.

Sure, but we won't commit= commented-out code. It just makes things messy until such time as it gets = used (or more often, does not).
=C2=A0
- On = reflection, I don't think the "Data saved successfully, you still = need to commit changes to the database." is prominent enough - in test= ing, I found it very easy to miss. That might also be compounded by the fac= t that "Alert on uncommitted transactions?" doesn't seem to b= e working for me. I get the "Save text" prompt to save the query = text, but if I say no to that, the Query Tool instance closes with no furth= er warning, despite having a transaction in progress.

I made a separate notification for the uncommitt= ed save to make it more visible, check it out.

That's definitely clearer now. It avoids the &quo= t;blindness" caused by the fact that you always get the green message.=
=C2=A0
I tried the scenario you pro= vided and the uncommitted alert worked fine, could you please try again and= tell me the exact scenario where that happened?

Hmm, it's working fine for me today too. Defini= tely wasn't yesterday though!

I'm going to= make some minor tweaks to the wording of the docs before I commit (as well= as removing the commented-out code), but I think this is good to go, once = it's had another review. Khushboo, please take a look as soon as you ca= n.

Thanks!=C2=A0

--
=
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--000000000000437974058d4ddb98--