Re: [GSoC] Finalized First Patch

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Dave Page <[email protected]>
To: Yosry Muhammad <[email protected]>
Cc: Khushboo Vashi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Akshay Joshi <[email protected]>
Subject: Re: [GSoC] Finalized First Patch
Date: Wed, 17 Jul 2019 11:46:26 +0100
Message-ID: <CA+OCxowFoexXkq-0Owb+0bEKSVPe4y+h3=BkR_OncS4rJ_XBoA@mail.gmail.com> (raw)
In-Reply-To: <CAFSMqn_AwMZYsjz5mGF7yTjxNX1N6_2Y9GeDO-Fza8pf_x4Y_Q@mail.gmail.com>
References: <CAFSMqn8yLA329GFHSm0Bn4cEqvd+kVoYpCAB5mRDbVF6NNaxxA@mail.gmail.com>
	<CAFSMqn8fjFT8CbhB5FAtkN8cd7QH0+XYtSUZmD5J1F38n1189g@mail.gmail.com>
	<CA+OCxoz=50F2N6+7dGrarieXNaXiiJw-deSAXd6fSH7SvEaQXw@mail.gmail.com>
	<CAFSMqn9HaapnutknGQdG2h0-xyk9qCcGB_wxkPM5iwiHp=fPpg@mail.gmail.com>
	<CA+OCxox43r3qH9YNt_gT7TT3s+DkROvf1pvvAKzg663xHzT5VQ@mail.gmail.com>
	<CAFOhELeFLVLS_RDTJPH0cLDRP8PUxFfxL05YXcL0+r1addP8dg@mail.gmail.com>
	<CAFOhELerRJ3WLau4rXVHvCC=YO9nEpp8qtX+f0tbq0rTqLO5uw@mail.gmail.com>
	<CAFSMqn_gEi1JyeHdeFqWKtjg9w4T8p7Edi8iKPEtX1_qZjo0Fg@mail.gmail.com>
	<CAFOhELdt8-dVFk-Jb9TFLahLYu5t-Lf6oNrK6u5HifLvgKGXNA@mail.gmail.com>
	<CAFSMqn_HLvsNAN4nQKX4qrea5vA8U0J87jGncyZ68hyQ24-3Jw@mail.gmail.com>
	<CAFOhELd0tha15rhhT_stYozm+ahXTrx6BF8bcydHHdye4eHEDw@mail.gmail.com>
	<CA+OCxoyOOwO1SLqgBW3Sx+nYEx2pYqgp==cU2be-G_EtVv4HZQ@mail.gmail.com>
	<CAFSMqn_27bbwSrme46k7L20UCDMcJ3-x22-5xGKW6bhPofpOKg@mail.gmail.com>
	<CAFSMqn_AwMZYsjz5mGF7yTjxNX1N6_2Y9GeDO-Fza8pf_x4Y_Q@mail.gmail.com>

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 <[email protected]> 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 <[email protected]>
> wrote:
>
>> I will look into it and get back to you.
>> Thanks !
>>
>> On Fri, Jul 12, 2019, 11:20 AM Dave Page <[email protected]> wrote:
>>
>>>
>>>
>>> On Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi <
>>> [email protected]> wrote:
>>>
>>>> Hi Yosry,
>>>>
>>>> On Thu, Jul 11, 2019 at 6:50 PM Yosry Muhammad <[email protected]>
>>>> 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 <
>>>>> [email protected]> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <[email protected]>
>>>>>> wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi <
>>>>>>> [email protected]> 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

view thread (19+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: [GSoC] Finalized First Patch
  In-Reply-To: <CA+OCxowFoexXkq-0Owb+0bEKSVPe4y+h3=BkR_OncS4rJ_XBoA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox