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 1hdpxM-0001jG-In for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Jun 2019 05:49:56 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hdpxK-0003uh-Q9 for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Jun 2019 05:49:54 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdpxK-0003ua-Hl for pgadmin-hackers@lists.postgresql.org; Thu, 20 Jun 2019 05:49:54 +0000 Received: from mail-lf1-x133.google.com ([2a00:1450:4864:20::133]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdpxF-0007Br-OF for pgadmin-hackers@postgresql.org; Thu, 20 Jun 2019 05:49:54 +0000 Received: by mail-lf1-x133.google.com with SMTP id d11so1478892lfb.4 for ; Wed, 19 Jun 2019 22:49:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mJvpLeBiHMDRiA00zLWbh472ME1WFeTn9+/6SWiKVNo=; b=DOAxd/IrYD0jUP6+E53etzGE3dREa01ZoWgIqDAZqYPKyddoeuq0sPB6wYqoiwNI7c K1WvbJ8ywbPA+XoM2gMEKG/f9TGVym19R/Sd92SXCaAd0wr9IvZzc68FdNkmm6PH1i6U PY3KQQ+bImWRg+ewjK/1Ezlbe+TqrUJ7fKFU3RbwcaeqWDPhHewZ8sQdYqP7PeTONOYA vq9wLNNqc8A1ztVkfKKReP7K840IBa5UM4DAu0AkFcazdvwVuQifqXtkb2VN5QzaTwjS LbiJCKBWSW+DcNx8iLZSFXbmh7gKICmE7tgjLLevoqa3Gg8zc+zbgMaOun8YfP3FvCmt kefg== 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:cc; bh=mJvpLeBiHMDRiA00zLWbh472ME1WFeTn9+/6SWiKVNo=; b=howmrskzUJDSAtPfD8ybtkJQxQUvca6KOL5bzewtJXf/0clZ38+u1k2dJOnXUB41H5 jV5XMS8g6abOp+VRKDVp+IqvCDe3zT5Gt9r0O3bkBnIPRzcUeKMW0++i8J0cg7g+8Fx1 JaQxYfJ5IHdMFIxuljn+Hx+UhSImPAWw0c43sKGTgqCj+BD2QisBAZ9tczMBoZM/ahet ZolRY7ldUKByJGdTVPIbzSLEnijsWWC/6h5u6UFro9wjelY8UkYA+wxXAJDVJOGU8xZ6 rdZqr1JUZkgwQev0saNTwkIrF2USGxjxRt9sn8hFqdiImJ/H4/xFagfgHEQJS+nI/lmu zyug== X-Gm-Message-State: APjAAAUknR9tEsr/0BY26v7O4rQ+Ih8AgEFxkRNR8WD/mZWAMppxTADV nqh+JWCRNBSLE9TIJKMLBV19sH/ReiXgo5vcKMrl7A== X-Google-Smtp-Source: APXvYqwYipzyd4fPbw8AhBEX1gK4B9xFPaS0KciHX55bvmYKSDQXi1F3NlvAuRhrpJ+ex/kQkYiJKsUcORVO8h+BF/o= X-Received: by 2002:a19:3f4b:: with SMTP id m72mr62804387lfa.91.1561009787568; Wed, 19 Jun 2019 22:49:47 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Aditya Toshniwal Date: Thu, 20 Jun 2019 11:19:11 +0530 Message-ID: Subject: Re: [GSoC][Patch] Automatic Mode Detection V1 To: Dave Page Cc: Yosry Muhammad , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000e117ae058bbae956" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000e117ae058bbae956 Content-Type: text/plain; charset="UTF-8" [forked the mail chain for code review] Hi Yosry, On Wed, Jun 19, 2019 at 5:24 PM Dave Page wrote: > Hi > > On Wed, Jun 19, 2019 at 6:18 AM Yosry Muhammad wrote: > >> >> Waiting for the icon, will set it up once it is ready. >> > > It's underway :-) > > >> I ran pep8 checks and JS tests on this patch, however I could not run >> python tests due to a problem with chromedriver (working on it), please let >> me know if any tests fail. >> > > Take a look in the Makefile (or web/regression/README) and you'll see how > you can run tests selectively - e.g. to avoid the feature tests when > running the Python suite, you can do "python regression/runtests.py > --exclude feature_tests" > > As for chromedriver, there's a utility (tools/get_chromedriver.py) you can > use to download and install the correct version. You should save it to > somewhere in your path; I'd suggest the bin/ directory in your virtual > environment. > > >> >> - When revising patches, please send an updated one for the whole thing, >>> rather than incremental ones. Incrementals are more work to apply and don't >>> give us any benefit in return. >>> >>> >> The attached patch is a single patch including all old and new increments. >> > > :-) > > Aditya; can you do a quick code review please? Bear in mind it's a work in > progress and there are no docs or tests etc. yet. > Nice work there. :) I just had look on the code changes, and have few suggestions: 1) I found the code around primary key in the function check_for_updatable_resultset_and_primary_keys repeating. Try if you cpuld modify/reuse the get_primary_keys function. 2) The function name check_for_updatable_resultset_and_primary_keys could be shorter, something like check_updatabale_rset_pkeys. Same for __set_updatable_resultset_attributes to __set_updatable_rset_attr 3) You've used background: #f4f4f4; for .highlighted_grid_cells class. This should go to sqleditor.scss with appropriate color from web/pgadmin/static/scss/resources/_default.variables.scss. Hard coded color codes are highly discouraged. Otherwise, looks good (didn't run and check) > > >> >> - 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. > > >> >>> - 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). > > >> >> >>> - 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. > > >> 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. > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- Thanks and Regards, Aditya Toshniwal Software Engineer | EnterpriseDB India | Pune "Don't Complain about Heat, Plant a TREE" --000000000000e117ae058bbae956 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
[forked the mail chain for code review]
<= div class=3D"gmail_default" style=3D"font-family:verdana,sans-serif">Hi Yos= ry,

On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dpage@pgadmin.org> wrote:
Hi

On Wed, Jun 19, 2= 019 at 6:18 AM Yosry Muhammad <yosrym93@gmail.com> wrote:

Waiting for the icon, will set it up= once it is ready.

=
It's underway :-)
=C2=A0
I ran pep8 checks and JS tests on this patch, however I could not run py= thon tests due to a problem with chromedriver (working on it), please let m= e know if any tests fail.

=
Take a look in the Makefile (or web/regression/README) and you&#= 39;ll see how you can run tests selectively - e.g. to avoid the feature tes= ts when running the Python suite, you can do "python regression/runtes= ts.py --exclude feature_tests"

As for chromed= river, there's a utility (tools/get_chromedriver.py) you can use to dow= nload and install the correct version. You should save it to somewhere in y= our path; I'd suggest the bin/ directory in your virtual environment.
=C2=A0

- When revising patches, please send an updated one f= or the whole thing, rather than incremental ones. Incrementals are more wor= k to apply and don't give us any benefit in return.


The attached patch is a singl= e patch including all old and new increments.

:-)

Aditya; can you do = a quick code review please? Bear in mind it's a work in progress and th= ere are no docs or tests etc. yet.
Nice work= there. :)
=C2=A0
I just had look on the code cha= nges, and have few suggestions:
1) I found the code around pri= mary key in the function check_fo= r_updatable_resultset_and_primary_keys repeating. Try if you cpuld modify/= reuse=C2=A0the get_primary_keys function.
2) The function name ch= eck_for_updatable_resultset_and_primary_keys could be shorter, something li= ke check_updatabale_rset_pkeys. Same for __set_updatable_resultset_attribut= es to __set_updatable_rset_attr
3) You've used background: #f4f4f4; = for .highlighted_grid_cells class. This should go to sqleditor.scss with ap= propriate color from web/pgadmin/static/scss/resources/_default.variables.s= css. Hard coded color codes are highly discouraged.
Otherwise, looks good (didn't run and check)<= /span>
=C2=A0

- We need to add a "do you want t= o 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 ro= ws are highlighted too.

<= /div>
Nice! I realise it's most likely not your code, but if you ca= n fix the wrapping so it doesn't break mid-word, that would be good. Se= e the attached screenshot to see what I mean.
=C2=A0

- If I m= ake two data edits and then delete a row, I get 3 entries in the History pa= nel, all showing the same delete. I would actually argue that data edit que= ries that pgAdmin generates should not go into the History at all, but mayb= e they should be added albeit with a flag to say they're internal queri= es and an option to hide them. Thoughts?

That was a bug with the existing 'save changes' act= ion of 'View Data', on which mine is based upon. I fixed the bug, n= ow the queries are shown correctly. However, the queries are shown in the f= orm 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, u= sername, text, created_at) VALUES (
%(media_url)s::character varying, %(= username)s::character varying, %(text)s::text, %(created_at)s::timestamp wi= thout time zone)
=C2=A0returning id;
=C2=A0
=
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 p= otentially helpful).
I like the idea of doing 2 - but I think we should have a check= box on the history panel to show/hide generated queries (and we should incl= ude transaction control - BEGIN, COMMIT etc - in the generated query class)= .
=C2=A0
<= div dir=3D"ltr">
=

- We need to think about h= ow data editing fits in with transaction control. Right now, it seems to ha= ppen 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 sugge= st 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 e= xisting behavior.
2- In Query Tool mode:
- If auto-comm= it 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 o= ngoing transaction (or a new one if no transaction is ongoing), they are no= t 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 pro= gress - especially as that can be the case after the user hits the Save but= ton and thinks their data is safe (a side-thought is that perhaps we should= n't require the Save button to be pressed when auto-commit is turned of= f, 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 t= o close the tab or the whole window.
=C2=A0
=
I think it makes more se= nse for filters to be disabled. I mean since the user is already writing SQ= L it would be more convenient to just edit it directly.

Well we're not going to just disable the= m - we'll either remove them, or try to make them work. I'm leaning= strongly towards just removing that code entirely.

=C2=A0
I meant disabling them in the q= uery 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.

--
Dave Page
Blo= g: http://pgsnake= .blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


--
Adi= tya Toshniwal
Software Engineer |=C2=A0EnterpriseDB India |=C2=A0Pune
"Don't Complain a= bout Heat, Plant a TREE"
 --000000000000e117ae058bbae956--