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

Hi Joao,

On Wed, Apr 25, 2018 at 6:55 PM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> 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.
>

I moved to another project so I didn't get a chance to look into the code
but a
s you are aware that ​we are no longer considering given the patch as a fix
for the issue instead someone from the team might fork the ​

​code and add the option in the library itself.​

Regards,
Murtuza

> @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
>>
>