On Thu, Jun 20, 2019 at 2:20 AM Yosry Muhammad <yosry...@gmail.com> wrote:
> Hi there ! > > On Wed, Jun 19, 2019 at 4:11 PM Dave Page <dp...@pgadmin.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 <dp...@pgadmin.org> wrote: > >> Hi >> >> On Wed, Jun 19, 2019 at 2:47 PM Yosry Muhammad <yosry...@gmail.com> >> wrote: >> >>> Hi, >>> >>> >>> On Wed, Jun 19, 2019, 1:54 PM Dave Page <dp...@pgadmin.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/ >
