Enable scrolling through available tags

[zpxp]

Enable scrolling through available tags by scrolling on the window title bar

Setting

; Scroll to cycle between available tags
enable-tags-scroll = false

Takes into account label-empty hiding empty tags

[mihirlad55]

Thanks for the pull request.

I was hesitant on adding scroll for tags initially before there are a lot more nuances to a tag-based window manager compared to a workspace-based i.e. i3. However, I think if we think this through properly, we can create an intuitive scroll feature.

A few things about this feature though:

• I added a few comments on the code
• I'd prefer the pull request to be to the feature-dwm branch. The master branch has been modified for public viewing (i.e. the README is completely modified). The feature-dwm branch is a much cleaner "fork" of polybar and is the one that an upstream pull request is open for. Also, I'd like to continue the flow of merges to happen towards master from the feature-dwm branch rather than have master merged into feature-dwm.
• We should think through the different cases of when a scroll would be attempted i.e. no tags selected, all tags selected, multiple tags selected, one tag selected, etc. and handle the scroll accordingly for each situation

Here's what I think would be the best way to do it: Instead of just checking if label-empty is an empty string, how about we add another option called tags-scroll-empty which when true allows you to scroll through empty tags? This way, you can have empty tags visible, but also scroll only through occupied ones if you would like. I think this would make the feature more flexible. This option would require a non-empty label-empty. We could throw a config error if label-empty is empty, but tags-scroll-empty is true, or we could just ignore the option.

There are several different cases to account for when a scroll might be attempted: tags-scroll-empty = false:

1. No tags selected: Cycle through occupied tags starting from first occupied tag
2. Single tag selected: Cycle through occupied tags starting from first selected tag
3. Multiple tag selected: Cycle through occupied tags starting from first selected tag
4. All tags selected: Cycle through occupied tags starting from first selected tag

tags-scroll-empty = true and <label-empty> is not empty:

1. No tags selected: Cycle through all tags starting from first tag
2. Single tag selected: Cycle through all tags starting from first selected tag
3. Multiple tag selected: Cycle through all tags starting from first selected tag
4. All tags selected: Cycle through all tags starting from first tag

I'd like to hear your thoughts about this.

[mihirlad55]

Ok I just force-pushed to fix the pull request, so the base of the commits is now feature-dwm

[zpxp]

There are several different cases to account for when a scroll might be attempted:
tags-scroll-empty = false:

No tags selected: Cycle through occupied tags starting from first occupied tag
Single tag selected: Cycle through occupied tags starting from first selected tag
Multiple tag selected: Cycle through occupied tags starting from first selected tag
All tags selected: Cycle through occupied tags starting from first selected tag
tags-scroll-empty = true and <label-empty> is not empty:

No tags selected: Cycle through all tags starting from first tag
Single tag selected: Cycle through all tags starting from first selected tag
Multiple tag selected: Cycle through all tags starting from first selected tag
All tags selected: Cycle through all tags starting from first tag

That sounds good. I can do that.

Question @mihirlad55 : Currently the scroll handler is just placed on the window title section, however if there is no window title, you cant scroll to change the tag. Is it possible to add the scroll event listener to the root of the bar or module so you can scroll on empty section of the bar?

[mihirlad55]

I actually commented about that in the code above. I wasn't sure if that was intentional. I was thinking to put the scroll handler on the tags which I think would make the most sense. To do this, the builder->cmd would have to be right underneath if (tag == TAG_LABEL_TAGS) and builder->cmd_close would have to be right at the end of the if statement.

If it was on the entire module, it would conflict with the layout scroll handler.

[zpxp]

@mihirlad55 Done

Thanks for the pull request.

I was hesitant on adding scroll for tags initially before there are a lot more nuances to a tag-based window manager compared to a workspace-based i.e. i3. However, I think if we think this through properly, we can create an intuitive scroll feature.

A few things about this feature though:

• I added a few comments on the code
• I'd prefer the pull request to be to the feature-dwm branch. The master branch has been modified for public viewing (i.e. the README is completely modified). The feature-dwm branch is a much cleaner "fork" of polybar and is the one that an upstream pull request is open for. Also, I'd like to continue the flow of merges to happen towards master from the feature-dwm branch rather than have master merged into feature-dwm.
• We should think through the different cases of when a scroll would be attempted i.e. no tags selected, all tags selected, multiple tags selected, one tag selected, etc. and handle the scroll accordingly for each situation

Here's what I think would be the best way to do it: Instead of just checking if label-empty is an empty string, how about we add another option called tags-scroll-empty which when true allows you to scroll through empty tags? This way, you can have empty tags visible, but also scroll only through occupied ones if you would like. I think this would make the feature more flexible. This option would require a non-empty label-empty. We could throw a config error if label-empty is empty, but tags-scroll-empty is true, or we could just ignore the option.

There are several different cases to account for when a scroll might be attempted: tags-scroll-empty = false:

1. No tags selected: Cycle through occupied tags starting from first occupied tag
2. Single tag selected: Cycle through occupied tags starting from first selected tag
3. Multiple tag selected: Cycle through occupied tags starting from first selected tag
4. All tags selected: Cycle through occupied tags starting from first selected tag

tags-scroll-empty = true and <label-empty> is not empty:

1. No tags selected: Cycle through all tags starting from first tag
2. Single tag selected: Cycle through all tags starting from first selected tag
3. Multiple tag selected: Cycle through all tags starting from first selected tag
4. All tags selected: Cycle through all tags starting from first tag

I'd like to hear your thoughts about this.

[mihirlad55]

Looks really good, but a few things I want to mention:

• I should've this mentioned in my previous comment, but I force-pushed to clean up the commit histories earlier. When you pulled the branch, I believe you didn't force pull which caused some ugly merges. I force-pushed again to fix it again. Make sure you force pull (git pull -f) the branch before you try committing to it, when someone force pushes to it.
• Try to keep your commit summaries under 50 characters in the future, its common convention and promotes more concise and thoughtful summary messages.
• I tested out the feature and played around with it a bit. It works for the most part, but it isn't very stable. Scrolling too fast or changing scroll directions rapidly can lead to a crash. I believe this might be because the library isn't thread-safe. Part of the issue is that every single scroll is going to send an IPC command which can overwhelm dwm with messages. In normal situations this isn't really an issue. I'm still going to think about this some more before I merge the pull request. I haven't decided yet if I should merge this feature now and worry about that issue later or fix the issue before merging this feature.
• What do you think about adding tags-scroll-reverse and tags-scroll-wrap while we're working on this feature? I'm fine with merging without those settings, but I just thought why not add it now rather than later.

[zpxp]

Ill wait until you decide if youre going to merge it before adding reverse and wrap scrolling.

[mihirlad55]

After playing around with it some more, when I try to quickly (at a speed I would in normal use) scroll to a certain tag, it tends to get stuck, sometimes crashing if its too fast. Maybe I just scroll too fast, lol. Also, it's not just the library, a high rate of messages being read/sent can cause the socket to time out. How does the scroll feel for you? Does it get stuck or crash in your normal use?

I think the best solution to this would be to add some sort of buffered command sending for scroll events, so if there's multiple scroll requests in x milliseconds, it counts the number of requests and then sends the "result" of those scroll requests as one command rather than sending each scroll request as a command.

[mihirlad55]

Ok after some more thought, I've decided this: I'll merge the pull request (I'll wait till you finish the wrap and reverse settings, if you want to do that).

Fixing the message rate problem is part of a larger problem with the module which may involve refactoring the input handling to have some sort of buffered output / rate limiting. I'll create a new issue to address this problem separately. You can carry on with this feature and ignore the problem for now.

[mihirlad55]

See issue #15

[zpxp]

Ok, ill do the wrap and reverse some time this week

[zpxp]

@mihirlad55 done

[mihirlad55]

Alright, I took a look, everything looks good. Again, I had to fix the PR by cherry picking the right commits and force pushing. If you haven't read what I earlier wrote about force-pushing/pulling, I do encourage you to read it. Anyway, thanks again for your pull request.