feat: Module Visibility #2320

parmort · 2020-12-20T18:01:50Z

What type of PR is this? (check all applicable)

Description

Creates new IPC commands to toggle the visibility of a specific module. End goal (as expressed in #2108) is to stop processing hidden modules, although I will probably complete an initial implementation to just hide the modules.

$ polybar-msg [-p PID] cmd hide.mymodule # Hides module mymodule
$ polybar-msg [-p PID] cmd show.mymodule # Shows module mymodule
$ polybar-msg [-p PID] cmd toggle.mymodule # Toggles visibility of mymodule

Related Issues & Documents

Closes #2108

Documentation (check all applicable)

This PR requires changes to the Wiki documentation (describe the changes)
This PR requires changes to the documentation inside the git repo (please add them to the PR).
Does not require documentation changes

patrick96 · 2020-12-20T19:37:05Z

Approach looks good. Having the visibility information inside the module should make it easier in the future to actually pause/unpause modules when they are invisible.

Btw. for all methods that you add to module_interface, you also need to add them to unsupported.hpp.

codecov · 2020-12-23T01:30:12Z

Codecov Report

Merging #2320 (0d6ef0e) into master (b49f325) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff            @@
##           master   #2320      +/-   ##
=========================================
- Coverage    7.85%   7.83%   -0.03%     
=========================================
  Files         142     142              
  Lines       10275   10303      +28     
=========================================
  Hits          807     807              
- Misses       9468    9496      +28

Flag	Coverage Δ
unittests	`7.83% <0.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
include/components/controller.hpp	`0.00% <ø> (ø)`
include/modules/meta/base.hpp	`0.00% <ø> (ø)`
include/modules/meta/base.inl	`0.00% <0.00%> (ø)`
src/components/controller.cpp	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b49f325...0d6ef0e. Read the comment docs.

patrick96

Good job!

Now that I see your implementation, I think it would be cleaner from a user perspective to be able to change module visibility through an action instead of a command, because then users could easily change module visibility through click actions instead of having to go through polybar-msg (which also targets all bars).

But we are not going to do this here, because there is no way for all modules to share an action without adding it to every module separately. I have some ideas about action routing (see this comment) which could make this easier in the future.

But it's still a good idea to merge this.

Could you also add the name of the ipc commands to the changelog?

src/components/controller.cpp

parmort · 2020-12-23T17:39:28Z

Would adding an additional config parameter to modules to determine default visibility be an idea? This would allow modules to be hidden by default.

[module/mymodule]
visible=true ; defaults true
; OR
hidden=false ; defaults false

This would be useful for, as an example, an email notification module which only appears when there are new/unread notifications.

But we are not going to do this here, because there is no way for all modules to share an action without adding it to every module separately. I have some ideas about action routing (see this comment) which could make this easier in the future.

This isn't the right place to have this discussion, but would overloads to the input function be an idea to fix this, similar to the signal system? Conservatively adding "universal" actions could allow the base module to define default functionality for certain actions, like hiding and showing modules.

patrick96 · 2020-12-26T22:18:49Z

This isn't the right place to have this discussion, but would overloads to the input function be an idea to fix this, similar to the signal system?

We already overload the input function, so yeah this could work, modules would just have to start calling the super function in their input methods.
I have already implemented a proof of concept of my action router idea in #2336. This makes the action code inside modules much cleaner (because they no longer have to differentiate between the different actions themselves). It would also make implementing this a lot cleaner.

patrick96

Code looks solid. I just recently moved the config.cmake file into its own folder and that caused a merge conflict you need to resolve before we can merge this.

include/modules/unsupported.hpp

include/modules/meta/base.inl

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

- Called in the module to maintain dependence on the signal emitter - Update CHANGELOG to make changes more verbose

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

patrick96

I went ahead and rebased against master so we can merge this.

I think having a module setting for the visible state is a good idea. Do you want to implement this in a new PR?

parmort · 2020-12-27T15:12:32Z

Yeah, I'll get working on that.

patrick96 · 2020-12-27T15:13:30Z

Awesome! Thanks 😃

patrick96 · 2020-12-27T21:30:00Z

I have now more or less finished my action router idea (#2336) and adding universal actions like the visibility change actions seems quite easy. The only downside is that we can't use toggle as the action name for toggling the module visibility because that name is already used for the date module's click action. We would have to come up with a different name, maybe toggle_visible.

parmort · 2020-12-27T22:49:38Z

Or we could change the date action to toggle-format or something. That would break configs though, so some sort of notification would be necessary, like a pinned issue or a note at the top of the readme.

patrick96 · 2020-12-27T23:09:37Z

The action names are the closest thing we have to a public API and I am not very keen on breaking it.

I think people can live with toggle-visible 😉

* Add option * Undo my havoc on the example config * Update CHANGELOG

patrick96 · 2021-01-08T18:24:15Z

People seem to like this feature ;)

https://www.reddit.com/r/Polybar/comments/kssoga/the_new_showhide_module_feature_is_amazing/

IPC commands are no longer necessary now that the actions are implemented. Changed some method permissions as well to reflect this.

* Add toggle_visible action * Add set_visible and set_invisible actions * Rename toggle_visible method to match `action_toggle_visible` -> `action_toggle_visibility` Matches with `EVENT_TOGGLE_VISIBILITY` * Update CHANGELOG * Revert #2320 IPC commands IPC commands are no longer necessary now that the actions are implemented. Changed some method permissions as well to reflect this. * Add logging and change action names - `module_toggle` - `module_show` - `module_hide` Delineate common actions to all modules with a `module_` prefix (for future actions too) * Update documentation

parmort changed the title ~~Module Visibility~~ [WIP] feat: Module Visibility Dec 20, 2020

parmort force-pushed the 2108-visibility branch from f96bcff to 2444c08 Compare December 23, 2020 00:13

parmort marked this pull request as ready for review December 23, 2020 01:31

parmort changed the title ~~[WIP] feat: Module Visibility~~ feat: Module Visibility Dec 23, 2020

parmort force-pushed the 2108-visibility branch from db39f08 to cdce000 Compare December 23, 2020 01:57

patrick96 requested changes Dec 23, 2020

View reviewed changes

src/components/controller.cpp Outdated Show resolved Hide resolved

src/components/controller.cpp Show resolved Hide resolved

patrick96 mentioned this pull request Dec 26, 2020

module: Implement action router #2336

Merged

9 tasks

patrick96 requested changes Dec 26, 2020

View reviewed changes

include/modules/unsupported.hpp Outdated Show resolved Hide resolved

include/modules/meta/base.inl Show resolved Hide resolved

parmort and others added 11 commits December 27, 2020 15:58

Hopefully implement visibility checking

b37adf8

Implement hide command

8cd3c39

Implement show and toggle commands

d09667e

Refactor and add some logging

15f2850

Run style checks and update CHANGELOG

c0be130

Get around unused parameter warnings

f6fb663

Change set_visible to return nothing

2595410

Make errors more informative

0b9f135

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

Update bar when changing module visibility

441a12e

- Called in the module to maintain dependence on the signal emitter - Update CHANGELOG to make changes more verbose

wrong var

ea4ce36

Update include/modules/unsupported.hpp

0d6ef0e

Co-authored-by: Patrick Ziegler <p.ziegler96@gmail.com>

patrick96 force-pushed the 2108-visibility branch from 09bffb8 to 0d6ef0e Compare December 27, 2020 15:00

patrick96 approved these changes Dec 27, 2020

View reviewed changes

patrick96 merged commit 8e46e54 into polybar:master Dec 27, 2020

parmort mentioned this pull request Jan 2, 2021

Add config option for visiblity (#2320 cont'd) #2342

Merged

9 tasks

parmort deleted the 2108-visibility branch January 2, 2021 21:35

patrick96 pushed a commit that referenced this pull request Jan 3, 2021

Add config option for visiblity (#2320 cont'd) (#2342)

eb302f9

* Add option * Undo my havoc on the example config * Update CHANGELOG

parmort mentioned this pull request May 9, 2021

Universal Visibility Action (#2320 cont'd) #2426

Merged

9 tasks

parmort added a commit to parmort/polybar that referenced this pull request May 9, 2021

Revert polybar#2320 IPC commands

9ba767c

IPC commands are no longer necessary now that the actions are implemented. Changed some method permissions as well to reflect this.

Uh oh!

feat: Module Visibility #2320

feat: Module Visibility #2320

Uh oh!

Conversation

parmort commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this? (check all applicable)

Description

Related Issues & Documents

Documentation (check all applicable)

Uh oh!

patrick96 commented Dec 20, 2020

Uh oh!

codecov bot commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

parmort commented Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick96 commented Dec 26, 2020

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

parmort commented Dec 27, 2020

Uh oh!

patrick96 commented Dec 27, 2020

Uh oh!

patrick96 commented Dec 27, 2020

Uh oh!

parmort commented Dec 27, 2020

Uh oh!

patrick96 commented Dec 27, 2020

Uh oh!

patrick96 commented Jan 8, 2021

Uh oh!

Uh oh!

parmort commented Dec 20, 2020 •

edited

Loading

codecov bot commented Dec 23, 2020 •

edited

Loading

parmort commented Dec 23, 2020 •

edited

Loading