Re: [GSoC] Finalized First Patch

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 <yosrym93(at)gmail(dot)com> wrote:

> I will look into it and get back to you.
> Thanks !
>
> On Fri, Jul 12, 2019, 11:20 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>>
>>
>> On Fri, Jul 12, 2019 at 5:57 AM Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Yosry,
>>>
>>> On Thu, Jul 11, 2019 at 6:50 PM Yosry Muhammad <yosrym93(at)gmail(dot)com>
>>> 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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> On Wed, Jul 10, 2019 at 3:11 PM Yosry Muhammad <yosrym93(at)gmail(dot)com>
>>>>> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Wed, Jul 10, 2019, 9:14 AM Khushboo Vashi <
>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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/