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 1hjM6q-0001kT-7k for pgadmin-hackers@arkaria.postgresql.org; Fri, 05 Jul 2019 11:10:32 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hjM6o-00013c-CF for pgadmin-hackers@arkaria.postgresql.org; Fri, 05 Jul 2019 11:10:30 +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 1hjM6n-00013V-Vx for pgadmin-hackers@lists.postgresql.org; Fri, 05 Jul 2019 11:10:30 +0000 Received: from mail-qt1-x844.google.com ([2607:f8b0:4864:20::844]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hjM6l-0003zs-9v for pgadmin-hackers@postgresql.org; Fri, 05 Jul 2019 11:10:28 +0000 Received: by mail-qt1-x844.google.com with SMTP id h18so3316930qtm.9 for ; Fri, 05 Jul 2019 04:10:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XmdPhzxSIvdOu9P4sWIHPoULSO7UG64cmAMAuEsk8w4=; b=uhQTs6J47d5silBGM5C5VN81dCxszam5MxFORGCmnKLgLOC0g6WCwvIzaFi0wxL4hW BEcg20yDD5CZP55o/EYnYqjHd6SnzeEOsJuyPEDrt8LVNF3+6BIDVX0ODWDYYt0V+aEX aqdChLdqcvV3qEzYOV185wQEfEBEyxEKJTVpirTU1DJR3JaKN67l0nYnJEbgAbI0jqut nzpQV85t3npltSULVdke5/FkMo/GWR+NlK+jej1fHVQRwb3utuRh6kPYdpbv/BQnM90I DeI0ZNIFS35Hc7dQVuvLK3Dsj0wrdZCT4H+2CbgHZIL+NNN1PRZqa0pyh/g3myVKE2QC Fn5Q== 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=XmdPhzxSIvdOu9P4sWIHPoULSO7UG64cmAMAuEsk8w4=; b=n5OuwKuRwWB0BNOxZhUwLRVivS5tzBSgXH9LR+FJTPdThrKgAI8g7b8HEe6HIzcghV 1PWzsNOlHS5jMDdIDPmxWUx6tiyq4zEuhFWGPK3gurVmozdiOC4V48expGcCTgs75OP2 cSkdc8vX6oqT8IbG+5f9o160FwUtUxJYssCaAOwUXBDyaUWapwH142VlaMGOcQVnzCpH iaOzqMqhKB4pre4Chz07G/82qsdGqQDjlkvy5LGjWTK8YKedS1pKU/nOOiLFedM9525K a0D2Mk/Ow71HM5qxPRVMd9Fa+BF6bQFGd/J7x/hbNYsoakqodyBGqPpz1kZUw0P6VJqJ wtkQ== X-Gm-Message-State: APjAAAUQXle/iE7ChLTtzECb3elIGhCRFdHsh9z42AdHXRkUeUmBGfkz Mi6yomy2EE2SZfLqhSG1Rxe3kEpDL9B4QkkqjrE= X-Google-Smtp-Source: APXvYqw5cWhk8Q2YaQuw98dHsKiOQ9qCEPl4C1Mdl2s6kx6Ck8M0QHPOAdXNARYUnHq/R6d+iM1enctIW0cDcCqtLPk= X-Received: by 2002:a0c:aff8:: with SMTP id t53mr2715589qvc.47.1562325026228; Fri, 05 Jul 2019 04:10:26 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yosry Muhammad Date: Fri, 5 Jul 2019 13:10:13 +0200 Message-ID: Subject: Re: [GSoC] Finalized First Patch To: Dave Page Cc: pgadmin-hackers@postgresql.org, Khushboo Vashi , Akshay Joshi Content-Type: multipart/alternative; boundary="000000000000364c40058ced249c" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000364c40058ced249c Content-Type: text/plain; charset="UTF-8" Sounds good ! Looking forward to any feedback. Thanks. On Fri, Jul 5, 2019, 1:01 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 > --000000000000364c40058ced249c Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Sounds good !=C2=A0

Looking=C2=A0forward to any feedback.

=
Thanks.

<= div dir=3D"ltr" class=3D"gmail_attr">On Fri, Jul 5, 2019, 1:01 PM Dave Page= <dpage@pgadmin.org> wrote:
Hi

=
On Fri, Jul 5, 2019 at 6:28 AM Yosry = Muhammad <yosrym93@gmail.com> wrote:
<= 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">
- The patch won't apply with "git apply" and o= nly partially applies with patch -p0. Please "git add" all your c= hanges and new files in your repo, and then run "git diff --cached --b= inary", 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.

Th= anks!
=C2=A0
=C2=A0
=
- e= xecute_query_utils.py is somewhat lacking in file header and any comments.<= br>

Added.

<= /div>
- 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 commi= t commented-out code. It just makes things messy until such time as it gets= used (or more often, does not).
=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">
- On= reflection, I don't think the "Data saved successfully, you still= need to commit changes to the database." is prominent enough - in tes= ting, I found it very easy to miss. That might also be compounded by the fa= ct 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 furt= her warning, despite having a transaction in progress.

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

That's definitely clearer now. It avoids the &qu= ot;blindness" caused by the fact that you always get the green message= .
=C2=A0
<= div dir=3D"ltr">
I tried the scenario you pr= ovided and the uncommitted alert worked fine, could you please try again an= d tell me the exact scenario where that happened?

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

I'm going t= o make some minor tweaks to the wording of the docs before I commit (as wel= l 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 c= an.

Thanks!=C2=A0

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB = UK: http://www.enterprisedb.com
The Enterpri= se PostgreSQL Company
--000000000000364c40058ced249c--