Re: [GSoC][Patch] Automatic Mode Detection V1

On Thu, Jun 20, 2019 at 2:20 AM Yosry Muhammad <yosrym93(at)gmail(dot)com> wrote:

> Hi there !
>
> On Wed, Jun 19, 2019 at 4:11 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>>
>>> - We need to think about how data editing fits in with transaction
>>> control. Right now, it seems to happen entirely outside of it - for
>>> example, I tend to work with auto commit turned off, so my connection sits
>>> idle-in-transaction following an initial select, and remains un-affected by
>>> edits. Please think about this and suggest options for us to discuss.
>>>
>>>
>>> I integrated the data editing in the transaction control as you noted.
>>> Now the behavior is as follows:
>>> 1- In View Data mode, same existing behavior.
>>> 2- In Query Tool mode:
>>> - If auto-commit is on: the modifications are made and commited once
>>> save is pressed.
>>> - If auto-commit is off: the modifications are made as part of the
>>> ongoing transaction (or a new one if no transaction is ongoing), they are
>>> not commited unless the user executes a commit command (or rollback).
>>>
>>>
>>> That seems to work. I think we need to make it more obvious that there's
>>> a transaction in progress - especially as that can be the case after the
>>> user hits the Save button and thinks their data is safe (a side-thought is
>>> that perhaps we shouldn't require the Save button to be pressed when
>>> auto-commit is turned off, as that's just odd). We should highlight the
>>> transaction state more clearly to the user, and make sure we prompt for
>>> confirmation if they try to close the tab or the whole window.
>>>
>>>
>>> The transaction status can be made more obvious and point out when a
>>> transaction is in progress that changes aren't commited. However, removing
>>> the save button when auto commit is off will cause us to a send a request
>>> and execute a query every time any cell is changed (which can be by
>>> accident or some kind of draft). I also think it will make more sense when
>>> there is a dedicated button, which can be named such that it is clear that
>>> it only executes some queries. Also, the pop up that shows after edits are
>>> succeesful can also state thar these changes are not yet commited.
>>>
>>
>> Yeah, I agree removing the button for some modes only is weird. Maybe
>> adding info to the notification, and having a more prominent "Your
>> transaction is still in progress" notification will be enough.
>>
>
> Will work on adding a notification and making the transaction status more
> prominent.
>
>
>>
>> Another thought - we also need to figure out what happens if the user
>> edits data in the grid and when saving, an error occurs (e.g. trying to
>> insert null into a not-null field). How does that tie into transaction
>> control? For example, auto-rollback may then revert other changes made via
>> SQL (which should have been atomic, with the data changes) - or having
>> auto-rollback turned off may then require the user to explicitly start a
>> new transaction before attempting to save the data again. Perhaps we need
>> to precede data changes with a savepoint, and then roll back to that if
>> there's an error?
>>
>
> I think the savepoint is an adequate solution.If any problems happen then
> rollback to the savepoint then release it, else just release the savepoint.
>
>
>> I think it makes more sense for filters to be disabled. I mean since the
>>> user is already writing SQL it would be more convenient to just edit it
>>> directly.
>>>
>>>
>>> Well we're not going to just disable them - we'll either remove them, or
>>> try to make them work. I'm leaning strongly towards just removing that code
>>> entirely.
>>>
>>>
>>> I meant disabling them in the query tool while keeping them in the View
>>> Data mode as the user cannot edit the sql in the View Data mode. Do you
>>> want to remove the feature from both modes completely?
>>>
>>>
>>> I think you misunderstand - I want to remove the View Data mode
>>> entirely. Your work should replace it.
>>>
>>>
>>> As a user of pgAdmin I think this might not be the best option. Not all
>>> users of pgAdmin are developers or know SQL. I worked on several projects
>>> before where other people on the team (or frontend developers) would just
>>> want to take a look at some data or do simple edits using the GUI. Also,
>>> other management studios for other DBMSs also allow for this. In addition,
>>> the user can do sorting of data without knowing SQL. What I think can be
>>> done (potentially - maybe in the future) is limit the dependance on SQL
>>> knowledge when doing filters in View Data mode, while disabling filters and
>>> so in the Query Tool.
>>>
>>
+1

View data mode is quick & easy way for any novice user who want to interact
with table data.

>> Hmm, the point of this project (which has been a goal for maybe 20
>> years!) was to remove that mode entirely. There is an argument that users
>> can use the "SELECT Script" option instead if they don't know SQL, but that
>> would still require the Sort/Filter options.
>>
>> What do other folks think?
>>
>
> Waiting for other people's opinion on that matter.
>
> Oh, and icon attached!
>>
>
> Will work on adding the new icon and switching the save functionality to
> it. I propose using this icon to save data in both View Data and Query Tool
> mode, and the existing save button for exclusively saving the query files
> (will be disabled in View Data mode).
>
> On Wed, Jun 19, 2019 at 4:11 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Wed, Jun 19, 2019 at 2:47 PM Yosry Muhammad <yosrym93(at)gmail(dot)com>
>> wrote:
>>
>>> Hi,
>>>
>>>
>>> On Wed, Jun 19, 2019, 1:54 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>> .
>>>
>>> - We need to add a "do you want to continue" warning before actions like
>>> Execute or EXPLAIN are run, if there are unsaved changes in the grid.
>>>
>>> - I think we should make the text in any cells that has been edited bold
>>> until saved, so the user can see where changes have been made (as they can
>>> with deleted rows).
>>>
>>>
>>> Both done, new rows are highlighted too.
>>>
>>>
>>> Nice! I realise it's most likely not your code, but if you can fix the
>>> wrapping so it doesn't break mid-word, that would be good. See the attached
>>> screenshot to see what I mean.
>>>
>>>
>>>
>>> Will do.
>>>
>>>
>>> - If I make two data edits and then delete a row, I get 3 entries in the
>>> History panel, all showing the same delete. I would actually argue that
>>> data edit queries that pgAdmin generates should not go into the History at
>>> all, but maybe they should be added albeit with a flag to say they're
>>> internal queries and an option to hide them. Thoughts?
>>>
>>>
>>> That was a bug with the existing 'save changes' action of 'View Data',
>>> on which mine is based upon. I fixed the bug, now the queries are shown
>>> correctly. However, the queries are shown in the form in which they are
>>> sent from the backend to the database driver (without parameters - also an
>>> already existing bug in View Data Mode), for example:
>>>
>>> INSERT INTO public.kweek (
>>> media_url, username, text, created_at) VALUES (
>>> %(media_url)s::character varying, %(username)s::character varying,
>>> %(text)s::text, %(created_at)s::timestamp without time zone)
>>> returning id;
>>>
>>>
>>> I propose two solutions:
>>> 1- Hide pgadmin's generated sql from history (in both modes).
>>> 2- Show the actual sql query that was executed after the parameters are
>>> plugged in (more understandable and potentially helpful).
>>>
>>>
>>> I like the idea of doing 2 - but I think we should have a checkbox on
>>> the history panel to show/hide generated queries (and we should include
>>> transaction control - BEGIN, COMMIT etc - in the generated query class).
>>>
>>>
>>>
>>> I can work on option 2 now and then work on
>>> the checkbox if/when there is time.
>>>
>>
>> I'm pretty sure there will be time :-)
>>
>>
>>>
>>>
>>>
>>> - We need to think about how data editing fits in with transaction
>>> control. Right now, it seems to happen entirely outside of it - for
>>> example, I tend to work with auto commit turned off, so my connection sits
>>> idle-in-transaction following an initial select, and remains un-affected by
>>> edits. Please think about this and suggest options for us to discuss.
>>>
>>>
>>> I integrated the data editing in the transaction control as you noted.
>>> Now the behavior is as follows:
>>> 1- In View Data mode, same existing behavior.
>>> 2- In Query Tool mode:
>>> - If auto-commit is on: the modifications are made and commited once
>>> save is pressed.
>>> - If auto-commit is off: the modifications are made as part of the
>>> ongoing transaction (or a new one if no transaction is ongoing), they are
>>> not commited unless the user executes a commit command (or rollback).
>>>
>>>
>>> That seems to work. I think we need to make it more obvious that there's
>>> a transaction in progress - especially as that can be the case after the
>>> user hits the Save button and thinks their data is safe (a side-thought is
>>> that perhaps we shouldn't require the Save button to be pressed when
>>> auto-commit is turned off, as that's just odd). We should highlight the
>>> transaction state more clearly to the user, and make sure we prompt for
>>> confirmation if they try to close the tab or the whole window.
>>>
>>>
>>> The transaction status can be made more obvious and point out when a
>>> transaction is in progress that changes aren't commited. However, removing
>>> the save button when auto commit is off will cause us to a send a request
>>> and execute a query every time any cell is changed (which can be by
>>> accident or some kind of draft). I also think it will make more sense when
>>> there is a dedicated button, which can be named such that it is clear that
>>> it only executes some queries. Also, the pop up that shows after edits are
>>> succeesful can also state thar these changes are not yet commited.
>>>
>>
>> Yeah, I agree removing the button for some modes only is weird. Maybe
>> adding info to the notification, and having a more prominent "Your
>> transaction is still in progress" notification will be enough.
>>
>> Another thought - we also need to figure out what happens if the user
>> edits data in the grid and when saving, an error occurs (e.g. trying to
>> insert null into a not-null field). How does that tie into transaction
>> control? For example, auto-rollback may then revert other changes made via
>> SQL (which should have been atomic, with the data changes) - or having
>> auto-rollback turned off may then require the user to explicitly start a
>> new transaction before attempting to save the data again. Perhaps we need
>> to precede data changes with a savepoint, and then roll back to that if
>> there's an error?
>>
>>
>>>
>>> I think it makes more sense for filters to be disabled. I mean since the
>>> user is already writing SQL it would be more convenient to just edit it
>>> directly.
>>>
>>>
>>> Well we're not going to just disable them - we'll either remove them, or
>>> try to make them work. I'm leaning strongly towards just removing that code
>>> entirely.
>>>
>>>
>>> I meant disabling them in the query tool while keeping them in the View
>>> Data mode as the user cannot edit the sql in the View Data mode. Do you
>>> want to remove the feature from both modes completely?
>>>
>>>
>>> I think you misunderstand - I want to remove the View Data mode
>>> entirely. Your work should replace it.
>>>
>>>
>>> As a user of pgAdmin I think this might not be the best option. Not all
>>> users of pgAdmin are developers or know SQL. I worked on several projects
>>> before where other people on the team (or frontend developers) would just
>>> want to take a look at some data or do simple edits using the GUI. Also,
>>> other management studios for other DBMSs also allow for this. In addition,
>>> the user can do sorting of data without knowing SQL. What I think can be
>>> done (potentially - maybe in the future) is limit the dependance on SQL
>>> knowledge when doing filters in View Data mode, while disabling filters and
>>> so in the Query Tool.
>>>
>>
>> Hmm, the point of this project (which has been a goal for maybe 20
>> years!) was to remove that mode entirely. There is an argument that users
>> can use the "SELECT Script" option instead if they don't know SQL, but that
>> would still require the Sort/Filter options.
>>
>> What do other folks think?
>>
>> Oh, and icon attached!
>>
>> --
>> 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/
>