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

Hi,
@Murtuza: We didn't notice the issue, can you please advise on what need to
change to make it work? The only change we did was to make one test pass.

@Hackers: In our point of view it is never good to fork a library. But if
he really have to do it, then we should fork it in Github, make our code
accessible to other people, and we should add it as a dependency on
package.json

Thanks
Anthony & Joao

On Wed, Apr 25, 2018 at 7:14 AM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> On Wed, Apr 25, 2018 at 12:13 PM, Murtuza Zabuawala <
> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave,
>>
>> On Wed, Apr 25, 2018 at 3:36 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> All,
>>>
>>> We just had a brief discussion in our EDB sprint planning meeting about
>>> this. There is a non-zero chance that we're going to have to fork wcDocker
>>> in the near future, in order to update it to work with jQuery 3. If we do
>>> that, then it may be significantly easier to fix this issue in that fork
>>> (perhaps by adding a single lockLayout(bool) function, rather than trying
>>> to do so from pgAdmin.
>>>
>>> I think (unless Murtuza believes that won't help), that we're better off
>>> holding on this for now until we know if we've had to do that.
>>>
>>
>> ​I don't have any objection forking the code and adding the flag to lock
>> the panel, But I'm certain that
>> we will use the same inbuilt method *panel.moveable(false)* which we
>> have used right now in the patch to prevent a panel from floating and will
>> face the same issue again which Akshay mentioned in his last email.
>>
>> Let me know if you want me to attach latest patch onto the ticket for
>> future reference and update the ticket accordingly​.
>>
>
> That's probably a good idea - thanks.
>
>
>>
>>
>>> On Wed, Apr 25, 2018 at 10:23 AM, Murtuza Zabuawala <
>>> murtuza(dot)zabuawala(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Akshay,
>>>>
>>>>
>>>> On Wed, Apr 25, 2018 at 2:37 PM, Akshay Joshi <
>>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Joao/Murtuza
>>>>>
>>>>> It break's the functionality, I am able to move "Data output",
>>>>> "Explain" etc.. panel of Query Tool, even if "Lock layout?" is set to True.
>>>>>
>>>>
>>>> ​It's working properly in v5 patch, Something went wrong while
>>>> refactoring.​
>>>>
>>>> ​
>>>>
>>>> Apart from above I have found more issue. Below are the steps to
>>>>> reproduce:
>>>>>
>>>>> - Set the "Lock layout?" flag to False.
>>>>> - Move out Dashboard panel.
>>>>> - Set the "Lock layout?" flag to True.
>>>>> - Close the Dashboard panel, as layout is locked and empty
>>>>> Dashboard panel is still visible. (Refer attached screenshot)
>>>>>
>>>>> ​That's because we have set the Panel moveable property to False, they
>>>> won't auto resize, As discussed earlier if user drag any panel out of panel
>>>> group it gets render in seprate wcFrame. I think that needs to be taken
>>>> care by user before they decide to lock the layout, We can not expilcitly
>>>> set panel's closeable property to False when layout is locked, If we do so
>>>> user will not be able to close any Query tool, Debugger panels.​
>>>> ​
>>>>
>>>>
>>>> On Tue, Apr 24, 2018 at 9:11 PM, Joao De Almeida Pereira <
>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>
>>>>>> haha,
>>>>>> Just joking, now we have a good version that passes tests and all.
>>>>>>
>>>>>> We found out that a test was failing in the patch version 5:
>>>>>>
>>>>>> HeadlessChrome 0.0.0 (Linux 0.0.0) Panel when we create a panel and user created panel without defining isMoveable then it should be moveable it should call moveable method with true as argument FAILED
>>>>>> Expected false to be true.
>>>>>> at UserContext.<anonymous> (regression/javascript/browser/panel_spec.js:12886:38)
>>>>>>
>>>>>> ​
>>>>>> To solve this problem we decided to change the Panel class to match
>>>>>> what the test say.
>>>>>>
>>>>>> Thanks
>>>>>> Victoria & Joao
>>>>>>
>>>>>>
>>>>>> On Tue, Apr 24, 2018 at 11:08 AM Joao De Almeida Pereira <
>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>
>>>>>>> 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>*
>>>>>>>>>
>>>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> *Akshay Joshi*
>>>>>
>>>>> *Sr. Software Architect *
>>>>>
>>>>>
>>>>>
>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91
>>>>> 976-788-8246 <+91%2097678%2088246>*
>>>>>
>>>>
>>>>
>>>
>>>
>>> --
>>> 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
>