Dock area fixes #469
Dock area fixes #469
Conversation
Codecov Report
@@ 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 |
|
I played around with this using 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.,
I wonder if the "x" should not be shown if it will have no behavior? |
|
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. |
|
@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). |
|
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. |
|
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
<#469 (comment)>, 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: ***@***.***>
|
|
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
force-pushed
the
dock-area-fixes
branch
from
01c2064
to
72db305
Compare
This allows to remove the item from the layout before closing it. Otherwise qt closes the widget backing the enaml declaration too early because it is parented to the layout which is destroyed when the floating window (which has no declarative equivalent) is destroyed.
This also fixes handling of placeholder item when the surrounding layout is not a splitter.
Previously the window would close as soon as the dock item is removed before it can be re-added as a pinned item.
|
@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. |
|
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
<#469 (comment)>, 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:
***@***.***>
|
|
At one point this will call for tests but I will merge as is for the time being. |
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.