Thread: [pgAdmin4][Patch]: To fix issues in Boolean editor

Hi,

PFA patch to fix the issue in boolean editor & also corrected feature test.

RM#2848

Hi

On Thu, Nov 16, 2017 at 2:04 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi,

PFA patch to fix the issue in boolean editor & also corrected feature test.

RM#2848

Testing in webkit on Mac (Qt 5.8), the problem does not appear to be solved for me. I have run make bundle, and restarted the runtime multiple times. 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

Hi Dave,

On Thu, Nov 16, 2017 at 8:59 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Nov 16, 2017 at 2:04 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi,

PFA patch to fix the issue in boolean editor & also corrected feature test.

RM#2848

Testing in webkit on Mac (Qt 5.8), the problem does not appear to be solved for me. I have run make bundle, and restarted the runtime multiple times. 

​I tested in Chrome and it was working as expected, Have you tested patch with browser?​

 

On Thu, Nov 16, 2017 at 4:30 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Thu, Nov 16, 2017 at 8:59 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Nov 16, 2017 at 2:04 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi,

PFA patch to fix the issue in boolean editor & also corrected feature test.

RM#2848

Testing in webkit on Mac (Qt 5.8), the problem does not appear to be solved for me. I have run make bundle, and restarted the runtime multiple times. 

​I tested in Chrome and it was working as expected, Have you tested patch with browser?​

 

It works fine in Chrome... but then, it was never broken there for me! 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

Hi Dave,

I wasn't working for me in Chrome, I almost have to click second time to set proper value in checkbox & even after that it was not saving the correct state properly.

And regarding issue in runtime, I checked with windows runtime and found that Qt does not support indeterminate state for the checkbox :(

So when we set the state to false it actually is indeterminate state and if we click one more time it sets it to false.

--

Regards,

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

On Thu, Nov 16, 2017 at 10:03 PM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Nov 16, 2017 at 4:30 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Thu, Nov 16, 2017 at 8:59 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Nov 16, 2017 at 2:04 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi,

PFA patch to fix the issue in boolean editor & also corrected feature test.

RM#2848

Testing in webkit on Mac (Qt 5.8), the problem does not appear to be solved for me. I have run make bundle, and restarted the runtime multiple times. 

​I tested in Chrome and it was working as expected, Have you tested patch with browser?​

 

It works fine in Chrome... but then, it was never broken there for me! 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

Hi

On Fri, Nov 17, 2017 at 6:46 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

I wasn't working for me in Chrome, I almost have to click second time to set proper value in checkbox & even after that it was not saving the correct state properly.

OK, well clearly that needs fixing too, but that wasn't the point of this particular ticket :-)

 

And regarding issue in runtime, I checked with windows runtime and found that Qt does not support indeterminate state for the checkbox :(

So when we set the state to false it actually is indeterminate state and if we click one more time it sets it to false.

OK, so how do we fix that? The Qt docs are quite clear that QCheckBox *does* support tri-state (see QCheckBox::setTriState(). Worst case is that we have to create our own control right?

 

--

Regards,

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

On Thu, Nov 16, 2017 at 10:03 PM, Dave Page <dpage@pgadmin.org> wrote:

On Thu, Nov 16, 2017 at 4:30 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

On Thu, Nov 16, 2017 at 8:59 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Thu, Nov 16, 2017 at 2:04 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi,

PFA patch to fix the issue in boolean editor & also corrected feature test.

RM#2848

Testing in webkit on Mac (Qt 5.8), the problem does not appear to be solved for me. I have run make bundle, and restarted the runtime multiple times. 

​I tested in Chrome and it was working as expected, Have you tested patch with browser?​

 

It works fine in Chrome... but then, it was never broken there for me! 

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

HI

On Tue, Nov 21, 2017 at 6:16 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

Cool - so a few thoughts...

- Can we make the null symbol a question mark?

- I think the feature tests should be enhanced to ensure we verify the clickthrough sequence of the new control. I don't think we need a new test - maybe just an enhancement to the existing editor test?

- With a table of the definition shown below, if I add a row with a default value for the ID, and false, true, null and hit save, then immediately (without refreshing) try to change the first boolean value to true and hit save, then I get the following error:

UPDATE public.bools SET

b1 = %(b1)s::boolean WHERE

;

2017-11-21 10:34:57,378: ERROR pgadmin:

Failed to execute query (execute_void) for the server #1 - DB:postgres

(Query-id: 4249364):

Error Message:ERROR:  syntax error at or near ";"

LINE 3: ;  

Table:

CREATE TABLE public.bools

(

    id integer NOT NULL DEFAULT nextval('bools_id_seq'::regclass),

    b1 boolean,

    b2 boolean,

    b3 boolean,

    CONSTRAINT bools_pkey PRIMARY KEY (id)

)

Thanks!

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

Hi Dave,

PFA updated patch.

On Tue, Nov 21, 2017 at 4:16 PM, Dave Page <dpage@pgadmin.org> wrote:

HI

On Tue, Nov 21, 2017 at 6:16 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

Cool - so a few thoughts...

- Can we make the null symbol a question mark?

​instead of question mark we will make square 

gray color

​.​

- I think the feature tests should be enhanced to ensure we verify the clickthrough sequence of the new control. I don't think we need a new test - maybe just an enhancement to the existing editor test?

​Done​

 

- With a table of the definition shown below, if I add a row with a default value for the ID, and false, true, null and hit save, then immediately (without refreshing) try to change the first boolean value to true and hit save, then I get the following error:

UPDATE public.bools SET

b1 = %(b1)s::boolean WHERE

;

2017-11-21 10:34:57,378: ERROR pgadmin:

Failed to execute query (execute_void) for the server #1 - DB:postgres

(Query-id: 4249364):

Error Message:ERROR:  syntax error at or near ";"

LINE 3: ;  

Table:

CREATE TABLE public.bools

(

    id integer NOT NULL DEFAULT nextval('bools_id_seq'::regclass),

    b1 boolean,

    b2 boolean,

    b3 boolean,

    CONSTRAINT bools_pkey PRIMARY KEY (id)

)

​This issue is not related to given editor changes.

I have opened the separate ticket for the issue 

https://redmine.postgresql.org/issues/2886 

Hi

On Tue, Nov 21, 2017 at 1:17 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch.

On Tue, Nov 21, 2017 at 4:16 PM, Dave Page <dpage@pgadmin.org> wrote:

HI

On Tue, Nov 21, 2017 at 6:16 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

Cool - so a few thoughts...

- Can we make the null symbol a question mark?

​instead of question mark we will make square 

gray color

​.​

Sorry - I just realised this is not good from an accessibility perspective as users may not be able to distinguish between the white and gray in-fill. I've made it include a ? as well. 

 

- I think the feature tests should be enhanced to ensure we verify the clickthrough sequence of the new control. I don't think we need a new test - maybe just an enhancement to the existing editor test?

​Done​

 

Thanks - patch applied.

 

- With a table of the definition shown below, if I add a row with a default value for the ID, and false, true, null and hit save, then immediately (without refreshing) try to change the first boolean value to true and hit save, then I get the following error:

UPDATE public.bools SET

b1 = %(b1)s::boolean WHERE

;

2017-11-21 10:34:57,378: ERROR pgadmin:

Failed to execute query (execute_void) for the server #1 - DB:postgres

(Query-id: 4249364):

Error Message:ERROR:  syntax error at or near ";"

LINE 3: ;  

Table:

CREATE TABLE public.bools

(

    id integer NOT NULL DEFAULT nextval('bools_id_seq'::regclass),

    b1 boolean,

    b2 boolean,

    b3 boolean,

    CONSTRAINT bools_pkey PRIMARY KEY (id)

)

​This issue is not related to given editor changes.

I have opened the separate ticket for the issue 

https://redmine.postgresql.org/issues/2886 

OK, thanks.  

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

​Hi Dave,

On Tue, Nov 21, 2017 at 10:53 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Tue, Nov 21, 2017 at 1:17 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch.

On Tue, Nov 21, 2017 at 4:16 PM, Dave Page <dpage@pgadmin.org> wrote:

HI

On Tue, Nov 21, 2017 at 6:16 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

Cool - so a few thoughts...

- Can we make the null symbol a question mark?

​instead of question mark we will make square 

gray color

​.​

Sorry - I just realised this is not good from an accessibility perspective as users may not be able to distinguish between the white and gray in-fill. I've made it include a ? as well. 

​

I have enhanced the the visibility of '?' 

little bit 

​more​

, attaching the patch if you prefer it that way.

 

 

- I think the feature tests should be enhanced to ensure we verify the clickthrough sequence of the new control. I don't think we need a new test - maybe just an enhancement to the existing editor test?

​Done​

 

Thanks - patch applied.

 

- With a table of the definition shown below, if I add a row with a default value for the ID, and false, true, null and hit save, then immediately (without refreshing) try to change the first boolean value to true and hit save, then I get the following error:

UPDATE public.bools SET

b1 = %(b1)s::boolean WHERE

;

2017-11-21 10:34:57,378: ERROR pgadmin:

Failed to execute query (execute_void) for the server #1 - DB:postgres

(Query-id: 4249364):

Error Message:ERROR:  syntax error at or near ";"

LINE 3: ;  

Table:

CREATE TABLE public.bools

(

    id integer NOT NULL DEFAULT nextval('bools_id_seq'::regclass),

    b1 boolean,

    b2 boolean,

    b3 boolean,

    CONSTRAINT bools_pkey PRIMARY KEY (id)

)

​This issue is not related to given editor changes.

I have opened the separate ticket for the issue 

https://redmine.postgresql.org/issues/2886 

OK, thanks.  

​Also attached patch for RM#2886, the issue was due to incorrect conditional logic, As per the current implementation the newly added row should gets disabled if it is saved with default primary key value until gird reloaded with actual PK for updation but due to

 incorrect condition

​ it was enable​ in this case.

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

​Hi Dave,

On Tue, Nov 21, 2017 at 10:53 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Tue, Nov 21, 2017 at 1:17 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch.

On Tue, Nov 21, 2017 at 4:16 PM, Dave Page <dpage@pgadmin.org> wrote:

HI

On Tue, Nov 21, 2017 at 6:16 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

Cool - so a few thoughts...

- Can we make the null symbol a question mark?

​instead of question mark we will make square 

gray color

​.​

Sorry - I just realised this is not good from an accessibility perspective as users may not be able to distinguish between the white and gray in-fill. I've made it include a ? as well. 

​

I have enhanced the the visibility of '?' 

little bit 

​more​

, attaching the patch if you prefer it that way. ​

 

 

 

- I think the feature tests should be enhanced to ensure we verify the clickthrough sequence of the new control. I don't think we need a new test - maybe just an enhancement to the existing editor test?

​Done​

 

Thanks - patch applied.

 

- With a table of the definition shown below, if I add a row with a default value for the ID, and false, true, null and hit save, then immediately (without refreshing) try to change the first boolean value to true and hit save, then I get the following error:

UPDATE public.bools SET

b1 = %(b1)s::boolean WHERE

;

2017-11-21 10:34:57,378: ERROR pgadmin:

Failed to execute query (execute_void) for the server #1 - DB:postgres

(Query-id: 4249364):

Error Message:ERROR:  syntax error at or near ";"

LINE 3: ;  

Table:

CREATE TABLE public.bools

(

    id integer NOT NULL DEFAULT nextval('bools_id_seq'::regclass),

    b1 boolean,

    b2 boolean,

    b3 boolean,

    CONSTRAINT bools_pkey PRIMARY KEY (id)

)

​This issue is not related to given editor changes.

I have opened the separate ticket for the issue 

https://redmine.postgresql.org/issues/2886 

OK, thanks.  

​Also attached patch for RM#2886, the issue was due to incorrect conditional logic, As per the current implementation the newly added row should gets disabled if it is saved with default primary key value until gird reloaded with actual PK for updation but due to

 incorrect condition

​ it was enable​ in this case.

--

Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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

On Wed, Nov 22, 2017 at 11:00 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

​Hi Dave,

On Tue, Nov 21, 2017 at 10:53 PM, Dave Page <dpage@pgadmin.org> wrote:

Hi

On Tue, Nov 21, 2017 at 1:17 PM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch.

On Tue, Nov 21, 2017 at 4:16 PM, Dave Page <dpage@pgadmin.org> wrote:

HI

On Tue, Nov 21, 2017 at 6:16 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:

Hi Dave,

PFA updated patch with custom tristate boolean editor for SlickGrid to make it compatible with Qt runtime.

I have tested it web mode & also modified the feature test accordingly.

Also thanks to Neel for testing the patch with latest runtime code.

Cool - so a few thoughts...

- Can we make the null symbol a question mark?

​instead of question mark we will make square 

gray color

​.​

Sorry - I just realised this is not good from an accessibility perspective as users may not be able to distinguish between the white and gray in-fill. I've made it include a ? as well. 

​

I have enhanced the the visibility of '?' 

little bit 

​more​

, attaching the patch if you prefer it that way.

In the call, Dave suggested that, we should have: 

Cross (×, i.e. &#215;) for False

Check (√, i.e. &#8730;) for True

Empty (∅, i.e. &#8709;) for Null.

We discussed it, which I think - Murtuza missed due to connectivity issue, during the call.

Dave - thoughts?