search

nucleic / enaml

Declarative User Interfaces for Python
http://enaml.readthedocs.io/en/latest/
Other
1.53k stars 130 forks source link

Dock area fixes #469

Closed MatthieuDartiailh closed 2 years ago

MatthieuDartiailh commented 2 years ago

While investigating and fixing #467, I found a bunch of other issues that this PR addresses along with #467.

I do not have time to wind up the unit tests this deserves so I would appreciate if anybody can stress test this is a bit before merging. Each commit addresses a specific issue and can be reviewed independently. Hopefully commit messages are clear.

codecov-commenter commented 2 years ago

Codecov Report

Merging #469 (72db305) into main (e9e1f30) will increase coverage by 0.01%. The diff coverage is 54.28%.

@@            Coverage Diff             @@
##             main     #469      +/-   ##
==========================================
+ Coverage   73.30%   73.31%   +0.01%     
==========================================
  Files         316      316              
  Lines       24104    24121      +17     
  Branches       55       55              
==========================================
+ Hits        17669    17684      +15     
- Misses       6435     6437       +2
bburan commented 2 years ago

I played around with this using psiexperiment and it seems to work fine. However, I did notice some additional behavior quirks with the behavior of the dock area that I had not noticed before. These quirks are present in main as well, so not a side-effect of this PR. For example:

In the following screenshot, there is an "x" indicating that we could close the tab group. Yet, it is not active because at least one item in the tab group cannot be closed (e.g., closable attribute is set to False). When all items can be closed, it does work (and closes them all out).

image

I wonder if the "x" should not be shown if it will have no behavior?

MatthieuDartiailh commented 2 years ago

Thanks for testing. We could either hide the x or close only all closable tabs and use a more specific tool tip. WDYT ?

Either way I will try to include the fix in this PR.

bburan commented 2 years ago

@MatthieuDartiailh I would argue for hiding the x since it seems counter-intuitive to show the x if it does not actually have the capability to close out all tabs (regardless of closability of each tab and/or the tooltip shown).

MatthieuDartiailh commented 2 years ago

Ok. I guess what I have in mind would make more sense as an entry in a context menu (I am thinking VS Code like, close all, close right, ...). But this I will leave this for later.

sccolbert commented 2 years ago

Another option would be to re-dock all the non-closable tabs to the main dock area when the X is clicked on a floating window.

On Fri, Jan 21, 2022 at 11:02 Matthieu Dartiailh @.***> wrote:

Ok. I guess what I have in mind would make more sense as an entry in a context menu (I am thinking VS Code like, close all, close right, ...). But this I will leave for later.

— Reply to this email directly, view it on GitHub https://github.com/nucleic/enaml/pull/469#issuecomment-1018691564, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABBQSOUIZEQ4MLY3AHYODLUXGGRBANCNFSM5LZII2DQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

MatthieuDartiailh commented 2 years ago

When closing a floating window non closable items or items whose closing is vetoed are re-floated and it works fine. The case of @bburan is specific to tabs floating or not and the handling of closing the whole notebook.

MatthieuDartiailh commented 2 years ago

@bburan I updated the code to hide the close button on the tabs if no item can be closed. This is closer to the old behavior while being more correct. I feel it could also be more convenient when there are many tabs and a single one cannot be closed. Let me know if you find anything else. I will plan to merge next week if nobody complains in the meantime.

bburan commented 2 years ago

That sounds great. Thank you.

On Sat, Jan 22, 2022 at 7:51 AM Matthieu Dartiailh @.***> wrote:

@bburan https://github.com/bburan I updated the code to hide the close button on the tabs if no item can be closed. This is closer to the old behavior while being more correct. I feel it could also be more convenient when there are many tabs and a single one cannot be closed. Let me know if you find anything else. I will plan to merge next week if nobody complains in the meantime.

— Reply to this email directly, view it on GitHub https://github.com/nucleic/enaml/pull/469#issuecomment-1019296211, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACPSQUA2OALNPORV2J5LZTUXLG6XANCNFSM5LZII2DQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

MatthieuDartiailh commented 2 years ago

At one point this will call for tests but I will merge as is for the time being.