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 1hdavs-0008G4-5p for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Jun 2019 13:47:24 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hdavo-00049F-BH for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Jun 2019 13:47:20 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdavn-000494-QY for pgadmin-hackers@lists.postgresql.org; Wed, 19 Jun 2019 13:47:20 +0000 Received: from mail-qk1-x733.google.com ([2607:f8b0:4864:20::733]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdavl-0003FT-4H for pgadmin-hackers@postgresql.org; Wed, 19 Jun 2019 13:47:18 +0000 Received: by mail-qk1-x733.google.com with SMTP id a27so10909872qkk.5 for ; Wed, 19 Jun 2019 06:47:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=wOuL1m35/8cTBJGZBDfmzuIDeNFBfAONIkzcacyxP1U=; b=N94/OqtZAHLtmvGTi7Jk+l1bJ1ZwzfmTtxljShzVnLAhZ4LKGT5/lwYb4ddambwkQK KHlFhKrYcM3/xvWuXgo0suAEIUTi55Xsgblasv3f7+1xwpruD4LwzdLdA5X/Mz6uRV61 ZfoDGPm0RXHxTMv/uqRn0t6hzZQZDJThBsKe9qbXrd1nrnvdprSzzqgcgzr64kj76BrN esX3YM+bjpE3WkNiJLIym9HRNy0XW0/GUnVG4MVwOw98jK75IBajEToTIMcFa1a76AnF hRZhk0YhXw3hGPfG2D4SI8Ypuk17EHaqXMeDybCqrqAQ8Uc53RAzkU0dAqQCZPTQOh85 4ztA== 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; bh=wOuL1m35/8cTBJGZBDfmzuIDeNFBfAONIkzcacyxP1U=; b=r3UTv1TnGjf/OHTvVl/TPzK3UwTWu2E8S1D6fdUXXuaqKhXeu3EF5HTQ6R5oCeV/Zy ByVKKFskZeEU8tzrPXwNrlKifItTxRzU7gHZsm9jT5eaK0QMs4UwvHdRJY/NIXdIunl3 WxQdHxic28vuVDYg8yKl/34SpRpNUxIS341lJevgtlYXRPfnvH6IODvCYqTNw8wvEIb7 bL1NSlmGhjmCtjNuS318kKbMSzhhF+jRqdFmRlzeFEXagin/eOIIhKEgEOuJhX0LpLf+ KSikJ668+4q822J6e+ghtsdFjqKwF2aK3ApxZEzj8ANZiWAZNLU/jczb+AyzaSUgD50E PBLg== X-Gm-Message-State: APjAAAVShkEJWa5ua3UJwkZ3R04uqm7YxiHWxUK8emCpP2oiQIR4rxEG hRANOsek7d3UQ1zvXT7cSjTJBoQ+/00GFMAvk//hWg== X-Google-Smtp-Source: APXvYqxJpYLyVtIJ88Yrh5YM2M19cvNFJRrg63P8zwCiDpMx4odHvv4cHbMAGaW3X2myzfdPjpZGX/drCH5ySwSOKnI= X-Received: by 2002:a37:b8c:: with SMTP id 134mr66791024qkl.160.1560952035775; Wed, 19 Jun 2019 06:47:15 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yosry Muhammad Date: Wed, 19 Jun 2019 15:47:04 +0200 Message-ID: Subject: Re: [GSoC][Patch] Automatic Mode Detection V1 To: Dave Page , pgadmin-hackers@postgresql.org Content-Type: multipart/alternative; boundary="0000000000009a9729058bad77ec" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000009a9729058bad77ec Content-Type: text/plain; charset="UTF-8" Hi, On Wed, Jun 19, 2019, 1:54 PM Dave Page 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. - 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. 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. Thanks ! --0000000000009a9729058bad77ec Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
H= i,


On Wed, Jun 19, 2019, 1:54 PM Dave Page <dpage@= pgadmin.org> wrote:
.
- We need to ad= d 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 e= dited bold until saved, so the user can see where changes have been made (a= s they can with deleted rows).

Both done, new rows are highlighted too.=C2=A0
<= /div>

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.
=C2=A0

<= /div>
Will do.


- If I make two data edits and= then delete a row, I get 3 entries in the History panel, all showing the s= ame delete. I would actually argue that data edit queries that pgAdmin gene= rates 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 hi= de them. Thoughts?

That w= as a bug with the existing 'save changes' action of 'View Data&= #39;, on which mine is based upon. I fixed the bug, now the queries are sho= wn 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:

<= div>
INSERT INTO= public.kweek (
media_url, username, text, created_at) VALUES (
%(med= ia_url)s::character varying, %(username)s::character varying, %(text)s::tex= t, %(created_at)s::timestamp without time zone)
=C2=A0returning id;
=C2=A0
I propose two solutions:
1- H= ide pgadmin's generated sql from history (in both modes).
2- = Show the actual sql query that was executed after the parameters are plugge= d 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 genera= ted queries (and we should include transaction control - BEGIN, COMMIT etc = - in the generated query class).
=C2=A0

I can work on option 2 now and then w= ork on=C2=A0
the checkbox if/when there is time.



- We need to think about how data editing fits= in with transaction control. Right now, it seems to happen entirely outsid= e of it - for example, I tend to work with auto commit turned off, so my co= nnection 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 th= e 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 modifi= cations are made and commited once save is pressed.
- If auto-com= mit 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 th= e user executes a commit command (or rollback).
=

That seems to work. I think we need to mak= e it more obvious that there's a transaction in progress - especially a= s 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 jus= t 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 t= he whole window.

=
The transaction status can be made more obvious and point out when a t= ransaction is in progress that changes aren't commited. However, removi= ng the save button when auto commit is off will cause us to a send a reques= t and execute a query every time any cell is changed (which can be by accid= ent 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 on= ly executes some queries. Also, the pop up that shows after edits are succe= esful can also state thar these changes are not yet commited.

I think it = makes more sense for filters to be disabled. I mean since the user is alrea= dy writing SQL it would be more convenient to just edit it directly.
<= /div>

Well we're not going to jus= t disable them - we'll either remove them, or try to make them work. I&= #39;m leaning strongly towards just removing that code entirely.
=
=C2=A0
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 featu= re 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 t= his might not be the best option. Not all users of pgAdmin are developers o= r know SQL. I worked on several projects before where other people on the t= eam (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 DB= MSs also allow for this. In addition, the user can do sorting of data witho= ut 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.

Thanks !
--0000000000009a9729058bad77ec--