Add tab visibility options: only unread tabs, live + unread tabs#6480
Add tab visibility options: only unread tabs, live + unread tabs#6480brilliantdrink wants to merge 5 commits intoChatterino:masterfrom
Conversation
|
Currently, there are no tests for tab visibility afaics. Creating those could be within the scope of this pr aswell. |
There was a problem hiding this comment.
UX-wise, I think right-clicking the tab menu would work well with a menu that looks like this:
Show all tabs (toggles all 3 checkboxes)
Hide all tabs (untoggles all 3 checkboxes)
[ ] Show live tabs
[ ] Show unread tabs
[ ] Show other tabs
Not sure - happy to bikeshed with the UX
For comparison, this is what the menu looks like now:
( ) Show all tabs
( ) Only show live tabs
( ) Hide all tabs
| * Obeys the HighlightsEnabled setting and the highlight state hierarchy and tracks the highlight state update sources | ||
| */ | ||
| void updateHighlightState(HighlightState style, | ||
| bool updateHighlightState(HighlightState style, |
There was a problem hiding this comment.
Update documentation with what the return value is meant to reflect
| state, split->getChannelView()); | ||
| if (this->tab_->updateHighlightState(state, split->getChannelView())) | ||
| { | ||
| if (auto *notebook = dynamic_cast<Notebook *>(this->parentWidget())) |
There was a problem hiding this comment.
Add a comment here - I imagine it's something along the lines of
"The highlight state of this tab changed, force refresh the notebook to ensure the tabs visibilities are recalculated"
src/widgets/Notebook.cpp
Outdated
| default: | ||
| this->setTabVisibilityFilter(nullptr); | ||
| break; | ||
| if ((visibility & (NotebookTabVisibility::LiveOnly | NotebookTabVisibility::UnreadOnly)) != 0) { |
There was a problem hiding this comment.
You could make use of our FlagsEnum class to make the comparisons a little bit more readable.
Additionally, if possible, I'd prefer if the actual visibility-callbacks for these states stay as simple as possible. Meaning if someone has set the filter to "live only", we should be able to use the { return tab->isLive(); } callback
If someone has set the filter to "unread only" we should be able to use the { return tab->highlightState() != HighlightState::None; } callback
a lot of yapping, but move as much of the logic out from the callback as possible, since it's run many times compared to the setting changing which is called less often
There was a problem hiding this comment.
Thanks! I've used the FlagsEnum in Notebook.cpp:1240 and Notebook.cpp:1314 for my new commit, I am not sure if I used it as intended.
I did attempt to change the type in the EnumSetting but it seems its not intended for that, or at least not yet made compatible (it is saying no type named 'type' in 'std::underlying_type<chatterino::FlagsEnum<chatterino::NotebookTabVisibilityFlag>>').
As for the visibility callback, I've changed it so that the flag check is outside the call back and only the tab-related checks are inside. I haven't found a way to make this non-redundant with the combination of flags yet, though.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| return NotebookTabVisibility::AllTabs; | ||
| return static_cast<std::underlying_type_t<NotebookTabVisibility>>(NotebookTabVisibility::AllTabs); | ||
| } |
There was a problem hiding this comment.
warning: do not use 'else' after 'return' [llvm-else-after-return]
| } | |
| if (args.value == "Only unread tabs") | |
| { | |
| return static_cast<std::underlying_type_t<NotebookTabVisibility>>(NotebookTabVisibility::UnreadOnly); | |
| } | |
| else if (args.value == "Live + unread tabs") | |
| { | |
| return NotebookTabVisibility::LiveOnly | NotebookTabVisibility::UnreadOnly; | |
| } | |
| else | |
| { | |
| return static_cast<std::underlying_type_t<NotebookTabVisibility>>(NotebookTabVisibility::AllTabs); | |
| } |
I just want to throw in that there was another request in #5027 with tab visibility "live + utility tabs". |
…bility check cb, add comments
|
Took a while, but here are some updates. I've updated the visibility submenu in the tab menu to look like this 
Selecting which tabs are shown feels to me like a filtering option, which is why i would opt not to add a "other" option to show all tabs, but rather solve it like this: those two options in the middle are not mutually exclusive, you can add and remove them like you would filters. Selecting to show all or to hide all does deselect all other options, though. If you deselect the hide option, it reverts to whatever was chosen before, e.g. if you select both unread and live, then select hide, and then click on "Hide all tabs" again it reverts to have unread and live selected. Deselecting "Show all tabs" does not work atm. I am seeing a bug still where changing the option inside settings does change the actual behaviour but does not update the list in the tab menu (which seems to be pre-existing but id say is within the scope of this pr) Also I think I have to take a look at |
Thanks for bringing that up! Ive had a look at it and it seems harder to implement than it seems. Afais, Chatterino doesn't really have a concept of "utility tabs", because every tab is just made up of multiple splits. You could check for each tab of any of the splits have a "utility" split, technically, but I would vouch for adding the option to pin single tabs so that they show regardless of the tab visibility setting as x9136 hinted at |
|
Okay, it turns out that not having a "other" option leads to ambiguous states, where "unchecking" The only problem I see now is in the settings page: tab visibility is now is conceptually a multi-select, but qt6 doesn't seem to have a multi select dropdown at all. Listing all permutations only works for two values, like in "Username style", but gets confusing. |
3 participants
This is my draft for adding options to show only unread tabs and live + unread tabs.
It changes how tab visibility is stored, from incremental enum values (0, 1, 2) to powers of two to use as a bit field. This allows combination of multiple "x only" options. Im thinking that I might be good to continue that logic to the input fields and convert it from a select menu to checkboxes.
closes #6475