Re: [pgAdmin4][RM#3155] Allow user to lock the Layout

From: Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>
To: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
Cc: Dave Page <dpage(at)pgadmin(dot)org>, Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4][RM#3155] Allow user to lock the Layout
Date: 2018-04-24 15:08:03
Message-ID: CAE+jja=p4WqZ=8sdONT0MEAR+LsStWGwpkSd2-NmwaHKP+d_-A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi,
Apparently the last version was not applying, here is the new version that
should apply correctly

Thanks
Victoria & Joao

On Tue, Apr 24, 2018 at 10:56 AM Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> Hi Murtuza,
>
> We tested the patch and everything looks fine. We also refactors some
> parts to include things like strict equality and using let/const instead of
> var. The updated patch is attached.
> In the future, it will be more valuable to have the translation to ES6 and
> the feature work in separate commits so it is easier to understand what
> changed.
>
> Sincerely,
>
> Joao and Victoria
>
>
>
> On Tue, Apr 24, 2018 at 4:58 AM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> On Tue, Apr 24, 2018 at 1:17 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Akshay, could you review/commit this please?
>>>
>>> Please also update the release_notes_3_1.rst file when you commit
>>> user-visible changes, to make it easier to build the release notes.
>>>
>>
>> Sure
>>
>>>
>>> Thanks.
>>>
>>> On Tue, Apr 24, 2018 at 8:45 AM, Murtuza Zabuawala <
>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave,
>>>>
>>>> Please find the updated patch, Now we are able to lock wcFrame and
>>>> wcPanel both.
>>>>
>>>> --
>>>> Regards,
>>>> Murtuza Zabuawala
>>>> EnterpriseDB: http://www.enterprisedb.com
>>>> The Enterprise PostgreSQL Company
>>>>
>>>>
>>>> On Thu, Apr 5, 2018 at 6:32 PM, Robert Eckhardt <reckhardt(at)pivotal(dot)io>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Wed, Apr 4, 2018 at 11:31 PM, Khushboo Vashi <
>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Apr 4, 2018 at 8:09 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Wed, Apr 4, 2018 at 12:54 PM, Murtuza Zabuawala <
>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> On Wed, Apr 4, 2018 at 5:00 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Apr 4, 2018 at 10:45 AM, Murtuza Zabuawala <
>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>
>>>>>>>>>> On Wed, Apr 4, 2018 at 2:47 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Apr 4, 2018 at 7:20 AM, Murtuza Zabuawala <
>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Dave,
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Apr 3, 2018 at 9:03 PM, Dave Page <dpage(at)pgadmin(dot)org>
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Hi
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Apr 3, 2018 at 12:56 PM, Murtuza Zabuawala <
>>>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks Joao for reviewing.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> PFA updated patch.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Apr 3, 2018 at 1:11 AM, Joao De Almeida Pereira <
>>>>>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, Apr 2, 2018 at 10:07 AM Murtuza Zabuawala <
>>>>>>>>>>>>>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ​Hello,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Please find updated patch,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Now layout will be locked after user updates its
>>>>>>>>>>>>>>>> preferences, w
>>>>>>>>>>>>>>>> e have used ​
>>>>>>>>>>>>>>>> templated variable in the javascript file
>>>>>>>>>>>>>>>> ​ because we do not have preference module or preference
>>>>>>>>>>>>>>>> cache available when the page loads and panels gets rendered,
>>>>>>>>>>>>>>>> ​I
>>>>>>>>>>>>>>>> ​ also
>>>>>>>>>>>>>>>> made changes in JS tests as per Joao's review comments.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Looks like everything is working when we change the lock.
>>>>>>>>>>>>>>> As a personal preferences I would prefer to see this in at
>>>>>>>>>>>>>>> least 2 commits, one that is related to the preference issue and another
>>>>>>>>>>>>>>> one that is related to this story.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> All the tests are working, but he linter is failing:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> /tmp/build/4a5630c2/pivotal-rm-3155/web /tmp/build/4a5630c2
>>>>>>>>>>>>>>> <https://gpdb-dev.bosh.pivotalci.info/teams/main/pipelines/pgadmin-feature-branches/jobs/pivotal-rm-3155-python-linter/builds/3#L5ab982d1:9>
>>>>>>>>>>>>>>> ./pgadmin/misc/__init__.py:78: [E303] too many blank lines (2)
>>>>>>>>>>>>>>> <https://gpdb-dev.bosh.pivotalci.info/teams/main/pipelines/pgadmin-feature-branches/jobs/pivotal-rm-3155-python-linter/builds/3#L5ab982d1:10>
>>>>>>>>>>>>>>> 1 E303 too many blank lines (2)
>>>>>>>>>>>>>>> <https://gpdb-dev.bosh.pivotalci.info/teams/main/pipelines/pgadmin-feature-branches/jobs/pivotal-rm-3155-python-linter/builds/3#L5ab982d1:11>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 1
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> ​Fixed​
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> @Dave/Pivotal team,
>>>>>>>>>>>>>>>> The given patch is working fine for all the Tabs/Panels
>>>>>>>>>>>>>>>> (all the panels from main window as well as from Query tool and Debugger)
>>>>>>>>>>>>>>>> but I'm facing an issue while handling the Browser tree section, It is a wcDocer
>>>>>>>>>>>>>>>> frame <http://docker.api.webcabin.org/module-wcFrame.html>
>>>>>>>>>>>>>>>> and not a wcDocker panel
>>>>>>>>>>>>>>>> <http://docker.api.webcabin.org/module-wcPanel.html>. Like
>>>>>>>>>>>>>>>> wcDocker panel, wcDocker frame do not provide any API so that a developer
>>>>>>>>>>>>>>>> can prevent drag-drop functionality on it.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>> It's not working fine for me. For example, if I put the SQL
>>>>>>>>>>>>> Panel on it's own below the properties/stats panels (so it looks like
>>>>>>>>>>>>> pgAdmin 3 used to by default), and then lock the layout, I can un-dock the
>>>>>>>>>>>>> SQL panel into a dialogue, but then cannot re-dock it. I can do weird
>>>>>>>>>>>>> things with the browser tree as well, probably because it's a frame as you
>>>>>>>>>>>>> say.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ​That is expected behaviour ​because once you drag the panel
>>>>>>>>>>>> out of the group of Panels then it becomes individual Frame, That is what
>>>>>>>>>>>> the author of the wcDocker replied on my question,
>>>>>>>>>>>> *"A panel must either be initialized as movable or non-movable
>>>>>>>>>>>> from the beginning and never changed because it generates a different
>>>>>>>>>>>> arrangement of elements depending. This feature should only ever be used
>>>>>>>>>>>> within the onCreate method of the panel. I should probably have been more
>>>>>>>>>>>> clear about this limitation in the documentation."*
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>> So does it become a panel again if a second panel is added to
>>>>>>>>>>> the new tab group?
>>>>>>>>>>>
>>>>>>>>>> ​No, it stays Frame.​
>>>>>>>>>>
>>>>>>>>>> As far as I understand Panel needs a Frame to render itself if it
>>>>>>>>>> is not attached to the main docker instance.​
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> There must be some way we can lock a tab that's not part of a
>>>>>>>>>>> group.
>>>>>>>>>>>
>>>>>>>>>> At a moment there is no way of ​
>>>>>>>>>> locking frames out of the box :(
>>>>>>>>>> ​
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hmm, so the question becomes: do we include the lock feature, but
>>>>>>>>> rename it to "Lock Tabs" or something similar, or leave it out altogether?
>>>>>>>>> It clearly doesn't do everything we want right now.
>>>>>>>>>
>>>>>>>> ​I would say lets include the feature by adding warning note that
>>>>>>>> this feature works with default layout only, And I don't think most user
>>>>>>>> will try to drag drop Browser panel ​
>>>>>>>> anyway, meanwhile I'll check what changes are required in main
>>>>>>>> source code to make the Frame lock.
>>>>>>>>
>>>>>>>
>>>>>>> Anyone else have any thoughts on this? Personally I don't like
>>>>>>> including half-baked features.
>>>>>>>
>>>>>>> +1, but we need to find out the way as this feature is requested by
>>>>>> many users.
>>>>>>
>>>>>
>>>>> 100% agree. I can convince my self that this feature request has to do
>>>>> with locking panels into a certain layout. I can also convince myself that
>>>>> the same request is simple because users are frustrated with the fact that
>>>>> the tabs and panes move around and they find that behavior annoying.
>>>>>
>>>>> -- Rob
>>>>>
>>>>>
>>>>>> --
>>>>>>> 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
>>>
>>
>>
>>
>> --
>> *Akshay Joshi*
>>
>> *Sr. Software Architect *
>>
>>
>>
>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 976-788-8246
>> <+91%2097678%2088246>*
>>
>

Attachment Content-Type Size
RM_3155_v7.patch text/x-patch 129.0 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Joao De Almeida Pereira 2018-04-24 15:41:05 Re: [pgAdmin4][RM#3155] Allow user to lock the Layout
Previous Message Joao De Almeida Pereira 2018-04-24 14:56:35 Re: [pgAdmin4][RM#3155] Allow user to lock the Layout