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 1hnhSP-0001G3-Vo for pgadmin-hackers@arkaria.postgresql.org; Wed, 17 Jul 2019 10:46:46 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hnhSO-00049D-OW for pgadmin-hackers@arkaria.postgresql.org; Wed, 17 Jul 2019 10:46:44 +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 1hnhSO-000496-DJ for pgadmin-hackers@lists.postgresql.org; Wed, 17 Jul 2019 10:46:44 +0000 Received: from mail-wm1-x343.google.com ([2a00:1450:4864:20::343]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hnhSL-0008L0-Lt for pgadmin-hackers@postgresql.org; Wed, 17 Jul 2019 10:46:44 +0000 Received: by mail-wm1-x343.google.com with SMTP id g67so17528576wme.1 for ; Wed, 17 Jul 2019 03:46:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4SmMffWasYYGVeCTT//oQddkbfNMFf9n81bTLZmLoZs=; b=fGLArRROAfz1CSlQbA9nIDRwTqBx8FIzHpllxKDRkb7N+8FiWbLhC5U9l0GPusFWi3 IDLmCliLdKYZAaESMLN5isnZrHLcsjefN//4iWYGB2rlnYcKouOLQYsnOS7LiPHg9CiQ Jmgbds7te4nHTjb+KAoJwZiGthwL9xdka6ebPBYFY8VaZcxAD1OWl0JzMssTXKQnP+m/ rymUARU1qPiknnVA+d9y5oq12yqh4CtnN50EwpwYlmmN+Akrqs8sz0PSGDFtZS5unlSg dmsTymBRNT0FcosUigRxXTfL70x0MNKLAXcg1J0+hTEmTHEb3NzUbLVoe3DNfXdQlKOt qNwg== 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=4SmMffWasYYGVeCTT//oQddkbfNMFf9n81bTLZmLoZs=; b=uDRkUmsa3m1xq+Wtk8fsToCkRyhCg2TImpqG3/Y7Ia29oP9GdMPFZfFXm4xtU4F95y G1gOfjSHZ6IPZJAXvoggSN41aa/Rq4I+BqpmMdBjkzzdDNljXKtT6M6EeQBpRocT3Ahk KTCQqBNoxCGJZckrLWPCZ74gL2ufGj+Aabzi+B6i15S+4H24cYx5YX+XZYpo3cu1/UPh CAwML6krvJDLk045hgelKucMwSjFqU7du9s50bPY6Aqu2/IpYZopyGmHyfXEwOZ792X6 i1O2tQIOcMDYgu22Ih0RFSz5pwO7u+pVwQekJnpewHAhVTtQ549p4aM+uR/hElkn/g59 PjpA== X-Gm-Message-State: APjAAAVuEAdubxYk6pJmaUfDwx5LZJm1m4OZRhKtZL44YmX3S0ps4O/e 7NXpr2S5cRVeBlmo5mBdmycrud5dE5/VUog43WSUfQ== X-Google-Smtp-Source: APXvYqzs+nHy3U9p5NIY3WL4iQhg6kBY8xoL9iXS6lXydW5JCpeAROW11e/9cEfOankfWBQdiz3av2SJSK3dn31Klkg= X-Received: by 2002:a7b:c0c6:: with SMTP id s6mr38189788wmh.115.1563360399262; Wed, 17 Jul 2019 03:46:39 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Wed, 17 Jul 2019 11:46:26 +0100 Message-ID: Subject: Re: [GSoC] Finalized First Patch To: Yosry Muhammad Cc: Khushboo Vashi , pgadmin-hackers , Akshay Joshi Content-Type: multipart/alternative; boundary="00000000000041191b058dde35ca" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000041191b058dde35ca Content-Type: text/plain; charset="UTF-8" Patch committed (with some trivial editorial work on the docs). Congratulations - that's impressive work! On Tue, Jul 16, 2019 at 6:03 AM Yosry Muhammad wrote: > Hi all, > > Please find attached an updated patch with the following modifications: > > - Fixed the bug noticed by Khushboo, it was caused because I was using the > connection status checked periodically to know the transaction status and > whether a transaction is active. Monitoring the connection status could be > disabled using preferences (which I assume was the case at Khushboo's). I > changed the code to store the last transaction status on any query > execution, polling, or saving of data changes. This transaction status is > used rather than the one checked periodically as it can be disabled. I also > refactored other parts of the code to make use of the stored transaction > status. > > - Created a new dialog that prompts the user to either commit or rollback > when exiting with an active transaction. In addition, the commit button is > disabled when the transaction is invalid (as the default behavior of > clicking commit when the transaction is invalid is rolling back, this makes > it clearer to the user that they can only rollback the transaction or > cancel the exit). Preferences and documentation where updated accordingly. > > - When the user performs a failed save as a part of an ongoing transaction > (with auto-commit off), a notification now clarifies that only the saving > action was rolledback rather than the whole transaction, therefore, there > previous queries are unaffected. > > Looking forward to your feedback. > Thanks ! > > > On Fri, Jul 12, 2019 at 11:53 AM Yosry Muhammad > wrote: > >> I will look into it and get back to you. >> Thanks ! >> >> On Fri, Jul 12, 2019, 11:20 AM Dave Page wrote: >> >>> >>> >>> On Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi < >>> khushboo.vashi@enterprisedb.com> wrote: >>> >>>> Hi Yosry, >>>> >>>> On Thu, Jul 11, 2019 at 6:50 PM Yosry Muhammad >>>> wrote: >>>> >>>>> Hi Khushboo, >>>>> Please find an updated patch attached with the mentioned import line >>>>> removed. >>>>> >>>>> Looks good to me. >>>> >>>>> On Thu, Jul 11, 2019 at 6:45 AM Khushboo Vashi < >>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad >>>>>> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi < >>>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>>> >>>>>>>> Some points I missed: >>>>>>>> 1. I assumed that in this patch modification in case of OIDs= True >>>>>>>> (without primary key) has not considered as that is not working. >>>>>>>> >>>>>>> >>>>>>> This is not implemented yet. I will work on that in a following >>>>>>> patch soon enough. >>>>>>> >>>>>>> Okay. >>>>>> >>>>>>> 2. As we are already showing the changed Data prompt on closing the >>>>>>>> Query Tool, do we really need the Uncommitted Transaction prompt? >>>>>>>> >>>>>>> >>>>>>> This is needed when auto-commit is off. Saving changes in the data >>>>>>> grid is performed as part of the ongoing transaction (or a new one if none >>>>>>> is ongoing). After saving the data changes the user should still commit the >>>>>>> current transaction for the changes to be commited to the database. This >>>>>>> feature is also useful in general when auto-commit is off as users may >>>>>>> forget to commit ongoing transactions. >>>>>>> >>>>>>> One thing I have noticed, when I add a new row and delete it >>>>>> immediately without saving it and try to close the query tool, the >>>>>> uncommitted prompt is coming. >>>>>> In my opinion, it should not come, what do you think? >>>>>> >>>>>> We should disable the prompt if auto-commit and auto-rollback both >>>>>> are enabled. >>>>>> >>>>> >>>>> The uncommited prompt does not keep track of what the user has done so >>>>> far, it only checks for the current transaction status. If a current >>>>> transaction is ongoing, the prompt comes up. If you added a new row then >>>>> deleted it without saving, the transaction status is not affected, you must >>>>> have done a previous operation and had auto-commit turned off (probably the >>>>> select statement). >>>>> if auto-commit & auto-rollback are both enabled then there won't be >>>>> any ongoing transaction at any point, thus, the prompt will never come up. >>>>> >>>>> Exactly, my point is. It should not prompt if auto-commit & >>>> auto-rollback both are enabled, but it is coming. Please see the attached >>>> video. >>>> >>> >>> Agreed - this should be fixed. >>> >>> If auto-commit is turned off, it should also prompt to commit if the >>> user hit Save in the prior step I think. Maybe reversing that prompt makes >>> more sense in general - prompting to save rather than discard is quite >>> normal; think about text editors etc. >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> > > -- > *Yosry Muhammad Yosry* > > Computer Engineering student, > The Faculty of Engineering, > Cairo University (2021). > Class representative of CMP 2021. > https://www.linkedin.com/in/yosrym93/ > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --00000000000041191b058dde35ca Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Patch committed (with some trivial editorial work on the d= ocs).=C2=A0

Congratulations - that's impressive work= !

On Tue, Jul 16, 2019 at 6:03 AM Yosry Muhammad <yosrym93@gmail.com> wrote:
Hi all,
<= div>
Please find attached an updated patch with the following= modifications:

- Fixed the bug noticed by Khushbo= o, it was caused because I was using the connection status checked periodic= ally to know the transaction status and whether a transaction is active. Mo= nitoring the connection status could be disabled using preferences (which I= assume was the case at Khushboo's). I changed the code to store the la= st transaction status on any query execution, polling, or saving of data ch= anges. This transaction status is used rather than the one checked periodic= ally as it can be disabled. I also refactored other parts of the code to ma= ke use of the stored transaction status.

- Cre= ated a new dialog that prompts the user to either commit or rollback when e= xiting with an active transaction. In addition, the commit button is disabl= ed when the transaction is invalid (as the default behavior of clicking com= mit when the transaction is invalid is rolling back, this makes it clearer = to the user that they can only rollback the transaction or cancel the exit)= . Preferences and documentation where updated accordingly.

- When the user performs a failed save as a part of an ongoing tra= nsaction (with auto-commit off), a notification now clarifies that only the= saving action was rolledback rather than the whole transaction, therefore,= there previous queries are unaffected.

Looking fo= rward to your feedback.
Thanks !

On Fri, = Jul 12, 2019 at 11:53 AM Yosry Muhammad <yosrym93@gmail.com> wrote:
I will look=C2= =A0into it and get=C2=A0back to you.
Thanks !
<= br>
On Fri,= Jul 12, 2019, 11:20 AM Dave Page <dpage@pgadmin.org> wrote:


On= Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi <khushboo.vash= i@enterprisedb.com> wrote:
Hi Yosry,

On Thu, Jul 11= , 2019 at 6:50 PM Yosry Muhammad <yosrym93@gmail.com> wrote:
<= /div>
Hi Khushboo,
Please find an updated patch = attached with the mentioned import line removed.

Looks good to me.=C2=A0=C2=A0
On Thu, Jul 11, 2019 at 6:45 AM Khushboo Vash= i <khushboo.vashi@enterprisedb.com> wrote:
=
Hi,

On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <yosrym93@gma= il.com> wrote:
Hi,

On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi = <khushboo.vashi@enterprisedb.com> wrote:
Some point= s I missed:
1.=C2=A0 I assumed that in this patch modification in case = of OIDs=3D True (without primary key) has not considered as that is not wor= king.

This is not implemented yet. I will work on that in a followin= g patch soon enough.

Okay.=C2=A0
<= div>2. As we are already showing the changed Data prompt on closing the Que= ry Tool, do we really need the Uncommitted Transaction prompt?=C2=A0
<= /div>

This is needed when auto-commit is off. Saving changes in the data grid is= performed as part of the ongoing transaction (or a new one if none is ongo= ing). After saving the data changes the user should still commit the curren= t transaction for the changes to be commited to the database. This feature = is also useful in general when auto-commit is off as users may forget to co= mmit ongoing transactions.

One thing I have noticed, when I add a new row and delete it immed= iately without saving it and try to close the query tool, the uncommitted p= rompt is coming.
In my opinion, it should not come, what do you t= hink?

We should disable the prompt if auto-commit = and auto-rollback both are enabled.

=
The uncommited prompt does not keep track of what the user has d= one so far, it only checks for the current transaction status. If a current= transaction is ongoing, the prompt comes up. If you added a new row then d= eleted it without saving, the transaction status is not affected, you must = have done a previous operation and had auto-commit turned off (probably the= select statement).
if auto-commit & auto-rollback are both e= nabled then there won't be any ongoing transaction at any point, thus, = the prompt will never come up.

Exactly, my point is. It should not prompt if auto-commit & = auto-rollback both are enabled, but it is coming. Please see the attached v= ideo.

Agreed - this shoul= d be fixed.=C2=A0

If auto-commit is turned off, it= should also prompt to commit if the user hit Save in the prior step I thin= k. Maybe reversing that prompt makes more sense in general - prompting to s= ave rather than discard is quite normal; think about text editors etc.
=C2=A0
--
= Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise = PostgreSQL Company


--
Yosry Muhammad Yosry

Compute= r Engineering student,
The Faculty of = Engineering,
Cairo University (2021).
Class representative of CMP 2021.


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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL = Company
--00000000000041191b058dde35ca--