highlight tabs only on unviewed messages #5649
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
So far I haven't noticed any performance issues either, although the decisions surely could be condensed into a precomputed NotebookTab<->NotebookTab, (except maybe the filter stuff, as I had to figure out to do highlight other tabs when a current tab shows filtering splits and a message is received but filtered out. i.e. not shown), the precomputed map could also be leveraged and used for 2) select tab unhighlight state change |
Co-authored-by: nerix <nero.9@hotmail.de>
|
for now selecting a tab correctly unselects other tabs based on their status of viewed channels as a whole - for new messages todo:
|
There was a problem hiding this comment.
From what I can tell, this works. Some test-cases would be great (not unit-tests, but written steps). For example:
Setup (could be the same for all tests):
- Tab 1: [TwitchDev]
- Tab 2: [TwitchDev]
- Tab 3: empty
Steps:
- Select Tab 3
- (some other account): Write
@{current-user}inTwitchDev(i.e. ping the current user) - expectation: Tab 1 and 2 are highlighted red
- Select Tab 1
- expectation: Tab 2 is no longer highlighted
If these get too long, you can use a <details> block (write /details and tab-complete):
<details><summary>Steps</summary>
text...
</details>
I'm unsure where I would even write these - (as a comment here?), or whats the verbiage on these actions. |
|
(Figured out what you meant by /details) Wrote out a bunch of test scenarios, hopefully I didn't confuse myself somewhere there. They should cover pretty much everything (Ofcourse Scenarios 4 and 5 - Multiple tabs - could test instead of just two, maybe three or four). Details
Common setup:
Scenario 1: (Messages in visible split)
Scenario 2: (Messages in nonvisible split)
Scenario 3: (Selecting single tab)
Scenario 4: (Selecting multiple tabs - same new state)
Scenario 5: (Selecting multiple tabs - different new states)
Scenario 6: (Creating new split)
Scenario 7: (Deleting a split)
Scenario 8: (Deleting a split and selecting other tab)
|
There was a problem hiding this comment.
one "bug": Two splits with the same filter applied in two separate tabs will both highlight with the current code
Setup:
Tab 1: [Moon with filter (message.content contains "forsen")]
Tab 2: [Moon with the same filter (message.content contains "forsen")]
Scenario:
Other user types "forsen" in Moon - both tabs highlight
not a final review, unit tests would be super handy but it's not something I think you should try to tackle in this PR
| * @brief Sets the highlight state of this tab clearing highlight sources | ||
| * | ||
| * Obeys the HighlightsEnabled setting and highlight states hierarchy | ||
| */ | ||
| void setHighlightState(HighlightState style); |
There was a problem hiding this comment.
Looks like setHighlightState is unused now (except for in unit tests) - should be removed then imo.
If you'd like, you can wait with this for a bit as I'd like to try to see if we can get ChannelView unit tests going
src/widgets/helper/NotebookTab.hpp
Outdated
| std::unordered_set<ChannelView::ChannelViewID> newMessageSource; | ||
| std::unordered_set<ChannelView::ChannelViewID> highlightedSource; |
There was a problem hiding this comment.
Document these members - I'm not sold on the naming of highlight source, so my thought is that with these documented to better describe your intent I might understand whether the name is ok or not
There was a problem hiding this comment.
the source in here refers to the split in which the message appears, I'll add this as a comment there
Not sure on which Tab youre actually selected, but if youre selected on a different Tab, e.g. Tab 3, then both Tab 1 and Tab 2 should highlight, then selecting either tab will unhiglight the other tab. Fixed the issue when you're on Tab 1 and Tab 2 gets highlighted, when it shouldn't (as you're viewing the same thing in Tab 1). I.e. each split (view) acts based on its own Identity (ID), i.e. having different filters will act as if its a different channel. |
There was a problem hiding this comment.
Nitpicky, but I think it will help me understand it more
I'd prefer if instead of storing "sources" of highlights, we store "unread highlights"
Instead of storing newMessage and highlighted unread highlights in separate sets, store them in a single map instead (e.g. map<ChannelViewID, HighlightState> where the HighlightState is the "highest" highlight state). If for some reason we must store both the new message and highlighted highlight states of an unread highlight, we could instead use a FlagsEnum, but I'd like to understand why that would be necessary first
Then, instead of having separate removeNewMessageSource etc it would be called markHighlightsFromChannelsAsRead(ChannelViewID) or something without having to specify the highlight type
let me know if there's anything in that feedback that doesn't make sense
My bad, read it wrong> Instead of storing `newMessage` and `highlighted` unread highlights in separate sets, store them in a single map instead (e.g. `map` where the HighlightState is the "highest" highlight state). If for some reason we must store both the `new message` and `highlighted` highlight states of an unread highlight, we could instead use a `FlagsEnum`, but I'd like to understand why that would be necessary first It is needed in this scenario Tab 1: [Earth, Moon] you're on Tab 4 [Mars] if you in channel #Earth get a new message (white) then Tab 1 shows as red (i.e. the highest state) If you select the Tab 3: [Moon] and view this ping message, then Tab 1 goes to the new message state (as you have yet to see the new message in channel #Earth Just state flags on a Tab would not work (as I see it) since you need to know who is the source of the highlight state (and as said above, also which state), to properly unmark them. Yes, I believe we only need the highest state of any given source, as it gets all removed on selection. So it might be possible, gonna take a look and try to implement it that way. |
incorporated the changes in 05ab3b2 |
|
Am I misunderstanding what this PR was supposed to do? from what I can see it still sets the duplicates of a tab as unread, even when you have one of them focused |
Are you using any filters in any of those tabs? Filter support with this is not perfect |
This draft tackles the problem of unnecessary higlighting of images viewed in currently selected notebook tab, as discussed here: #5412
It only tackles the part 1) do not highlight other tabs on messages shown in current tab, does not yet tackle the part 2) on selection unhiglight possible other tabs.
This is just a proof of concept, just to see what might be necessary to do these changes and to tackle the ideas.