Use module name in action string #1907
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
I have now made all modules input handlers, easier this way. But the controller still stores the input handlers, in case we want to introduce other kinds of input handlers in the future. I also removed the error for non unique module names and now deliver inputs to all matching input handlers. |
Member
Author
|
Hey @Lomadriel, could I get some feedback on this to see if you think I'm going in the right direction. Or do you see any issues with this? |
Member
The implementation seems quite good to me. I'll check that ASAP. For the idea itself, I really like your modification about the unique name. I think you are going in the right direction 👍 |
patrick96
added a commit
to patrick96/polybar
that referenced
this pull request
Jan 7, 2020
Functionality-wise reverts the changes from polybar#1534 In polybar#1907 we have decided to allow the same module to appear multiple times (and deliver actions to all matching modules). But since that PR will likely take longer to get merged, I want to remove the error from polybar because the message it prints isn't really true anymore.
patrick96
added a commit
that referenced
this pull request
Jan 12, 2020
Functionality-wise reverts the changes from #1534 In #1907 we have decided to allow the same module to appear multiple times (and deliver actions to all matching modules). But since that PR will likely take longer to get merged, I want to remove the error from polybar because the message it prints isn't really true anymore.
Member
Author
|
Quick status update: I have now switched over all the modules to support this new format. I have also renamed many of the actions using more inuitive and consistent names. I have not yet tested any of this functionality. I also want to look into how much effort it would take to write some unit tests here. Right now the menu module is a bit broken because before it intercepted all actions that polybar received (this includes shell commands) and compared them to its exec commands, if one of them matched, it closed the menu and tell the controller that it doesn't support that command. I am now working on forwarding all the legacy action names to the proper place. There are also actions that take data (for example which workspace to focus), I think we should make this data a part of the action specification. |
Lomadriel
reviewed
May 11, 2020
| using namespace modules; | ||
|
|
||
| namespace { | ||
| module_interface* make_module(string&& name, const bar_settings& bar, string module_name, const logger& m_log) { |
Member
There was a problem hiding this comment.
(Not for this PR)
We should use a sort of global unordered_map and register each module in a map
This function is so ugly...
Member
Author
There was a problem hiding this comment.
I agree. I tried to make it a bit nicer in this PR by moving the type string into the module header but still.
But I'm not really sure how to achieve this, C++ doesn't have classes at runtime (only instances), so you can't do something like this: clazz.newInstance() as you can do in java.
Member
There was a problem hiding this comment.
You can do something like:
/* General code*/
struct BaseModuleFactory {
BaseModuleFactory() { /* do stuff */ }
virtual ModuleInterface* build(...) = 0;
};
template <typename T>
struct ModuleFactory : BaseModuleFactory {
ModuleInterface* build(...) override { return new T(...); }
};
/* In module code */
ModuleFactory<MyModule> var;
Member
Author
There was a problem hiding this comment.
Oh, and then we map module types to ModuleFactory instances?
Member
There was a problem hiding this comment.
Yeap
// Factory header
void registerModuleFactory(std::string name, BaseModuleFactory& f);
// Factory source
static std::unordered_map<std::string, BaseModuleFactory&>& getFactories(); // Returns factory map to avoid static order initialization fiasco
void registerModuleFactory(std::string name, BaseModuleFactory& f) {
getFactories()[name] = f;
}
Member
Author
There was a problem hiding this comment.
Neat! We should definitely look into that. But maybe after this PR is merged.
8f17e21 to
0bfc8aa
Compare
The next action should always select the next workspace, the same for prev. reverse-scroll should be directly used when setting the scroll actions. This changes the behavior of `prev` and `next` actions in the i3 and bspwm module. But I don't think the impact will be significant and the old behavior was misleading anyway.
Only modules can now be action handlers. This also slightly simplifies the controller because we don't need to keep track of input handlers, we can just use the module list.
Member
Author
|
I believe I am now finally finished. From what I can tell everything works correctly. I think how actions are delivered to modules could still be cleaned up a bit. For example, each module could register itself as well as all its actions to an action router together with whether the action takes data and a reference to a member function that should be called when that action is triggered. This would remove the logic from all modules that determines which action should be executed. But we should leave this for a future PR, the changes here are enough. |
Codecov Report
@@ Coverage Diff @@
## master #1907 +/- ##
=========================================
+ Coverage 6.92% 7.09% +0.17%
=========================================
Files 153 154 +1
Lines 10482 10568 +86
=========================================
+ Hits 726 750 +24
- Misses 9756 9818 +62
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
patrick96
added a commit
that referenced
this pull request
Nov 29, 2020
Breaking Changes:
* The new config parser imposes some restrictions on which characters can be
used in section and key names. Users shouldn't be affected by this unless they
use one of the following characters in any section or key name: `"'=;#[](){}:.$\%`
Please consult [`man 5 polybar`](https://polybar.readthedocs.io/en/latest/man/polybar.5.html) for a full reference of the new config rules.
* `internal/temperature`: The first and last ramp element are now only used for
`base-temperature` and below and `warn-temperature` and above respectively.
This only slightly changes the ranges for which the different ramp levels are
responsible for.
(#2197)
Changelog
**Deprecations**
* `[settings]`: `throttle-input-for` has been removed. It wasn't a useful option
and could cause certain actions (e.g. click commands) to be ignored. (#2117)
* All action names used by modules to handle click and scroll events are
depercated (#1907). This change mainly affects users of the menu module.
Please read the [documentation](https://polybar.readthedocs.io/en/latest/user/actions.html) for instructions on how to migrate.
**New Config Options**
The `include-directory` key can be used the same as `include-file` and includes
all regular files in the given directory.
In labels:
* `label-NAME-minlen`, `label-NAME-alignment` can be used to pad labels with
spaces to the right (alignment set to `left`), left (alignment set to
`right`), and both sides (alignment set to `center`).
In `internal/backlight`:
* `enable-scroll` enables changing the brightness through scrolling.
In `internal/github`:
* `format-offline` is used when the module cannot connect to the server.
* `label-offline` can be used in `format-offline`.
* `api-url` can be used to connect to a custom github enterprise instance
In `internal/pulseaudio`:
* `click-right` and `click-middle` can be used to execute commands when
right/middle clicking.
* `%decibels%` token can be used in `label-volume` and `label-muted` to show the
volume in decibels.
**Changes To The Build System**
* Allow users to specify python executable when building. (polybar/xpp#27,
#2125)
* The i3ipcpp submodule no longer rebuilds jsoncpp and just uses whatever
version is available. (#2015, polybar/i3ipcpp#9)
**Features**
* New commandline argument: `-M` / `--list-all-monitors`.
Will display all available monitors (even cloned ones).
* New log level: `notice`.
Used as the default and is used for non-warning messages the user should
nevertheless be aware of. (#2027)
* config:
* New config parser (#1377)
* `include-directory` key (#2196), see #1946
* Add error message when an entire section is missing. (#2195)
* `-minlen` and `-alignment` properties for labels. (#1546)
* Make the `seperator` key in the bar section a label. (#1918)
* Better color validation. (#1290)
* timer modules: Schedule module updates to be aligned with the update interval.
For example, the date module now updates on the minute instead of in the
middle of a minute if the interval is set to 60 seconds. (#2123), see #2064
* `custom/menu`: Multiple menu modules per bar (#1907)
* `internal/backlight`: Support for changing the brightness through scrolling.
This may require additional changes to the system running polybar. (#1957)
* `internal/github`:
* `format-offline` for when the module cannot connect to the server (#1825),
see #1815
* Support for github enterprise instances. (#1841), see #1812
* `internal/network`: Support `Gbit/s` for `%linkspeed%` token. (#2055)
* `internal/pulseaudio`:
* `click-right` and `click-middle` keys (#1941)
* `%decibels%` token (#1894), see #1886
* `internal/xworkspaces`: Proper implementation for `label-occupied`. (#822),
see #874, #1444, #1033
**Fixes**
* Polybar not executing commands that produce output. (#1680), see #916
* Polybar froze until click commands finished executing (#2248)
* Polybar not properly working with mirrored monitors. (#1823), see #1192 and
#1794
* Unstable animation framerate (#1683), see #568
* Multiple modules of the same type caused click events not to be delivered to
the rigth one (#1907), see #1172
* config:
* Seemingly unrelated error messages when BOM character appears in config.
(#2166), see #2075
* Fall back to next possible config location if config file doesn't exist
(except if `--config` was used). (#2026), see #2016
* iconset: `fuzzy-match` chose first match, even if exact match was available.
(#2042), see #2041
* `custom/menu`: Spacing issue (#1656)
* `internal/alsa`: Volume didn't go over 100% (#2184), see #2173
* `internal/backlight`: Use amdgpu workaround for all devices starting with
`amdgpu_bl`. (#2122)
* `internal/battery`: Battery not marked as full if over `full-at` percent.
(#2019), see #1622
* `internal/cpu`: More accurate cpu load calculation. (#1955)
* `internal/github`: Outdated GitHub API authentication. (#2029), see #2002
* `internal/memory`: Use the correct size prefixes (#2211), see #2023
* `internal/network`:
* Wrong up- and downspeed values for non-integer intervals. (#2219)
* tun/tap interfaces didn't query their IP addresses. (#1990), see #1986
* Don't crash module if linkspeed cannot be queried. (#1772), see #1211
* `internal/temperature`:
* `format-warn` was not used if the temperature was *exactly*
`warn-temperature`. (#1897)
* `internal/xworkspaces`:
* Multi-monitor issue (#1929), see #1764, #1849
* Module sometimes showed too many workspaces (#1984), see #1983
* build:
* xpp submodule doesn't work with python 3.9 (polybar/xpp#26)
* CMake 3.17+ developer warnings (#2089, polybar/xpp#24, polybar/xpp#25)
* gtest compilation failure (#1993), see google/googletest#2678
* Compilation issue in GCC 6. (#1953)
patrick96
added a commit
that referenced
this pull request
Dec 1, 2020
Breaking Changes:
* The new config parser imposes some restrictions on which characters can be
used in section and key names. Users shouldn't be affected by, this unless
they use one of the following characters in any section or key name: `"'=;#[](){}:.$\%`
Please consult [`man 5 polybar`](https://polybar.readthedocs.io/en/latest/man/polybar.5.html) for a full reference of the new config rules.
* `internal/temperature`: The first and last ramp element are now only used for
`base-temperature` and below and `warn-temperature` and above respectively.
This only slightly changes the ranges for which the different ramp levels are
responsible for.
(#2197)
Changelog
**Deprecations**
* `[settings]`: `throttle-input-for` has been removed. It wasn't a useful option
and could cause certain actions (e.g. click commands) to be ignored. (#2117)
* All action names used by modules to handle click and scroll events are
deprecated (#1907). This change mainly affects users of the menu module.
Please read the [documentation](https://polybar.readthedocs.io/en/latest/user/actions.html) for instructions on how to migrate.
**New Config Options**
The `include-directory` key can be used the same as `include-file` and includes
all regular files in the given directory.
In labels:
* `label-NAME-minlen`, `label-NAME-alignment` can be used to pad labels with
spaces to the right (alignment set to `left`), left (alignment set to
`right`), and both sides (alignment set to `center`).
In `internal/backlight`:
* `enable-scroll` enables changing the brightness through scrolling.
In `internal/github`:
* `format-offline` is used when the module cannot connect to the server.
* `label-offline` can be used in `format-offline`.
* `api-url` can be used to connect to a custom github enterprise instance
In `internal/pulseaudio`:
* `click-right` and `click-middle` can be used to execute commands when
right/middle clicking.
* `%decibels%` token can be used in `label-volume` and `label-muted` to show the
volume in decibels.
**Changes To The Build System**
* Allow users to specify python executable when building. (polybar/xpp#27,
#2125)
* The i3ipcpp submodule no longer rebuilds jsoncpp and just uses whatever
version is available. (#2015, polybar/i3ipcpp#9)
**Features**
* New commandline argument: `-M` / `--list-all-monitors`.
Will display all available monitors (even cloned ones).
* New log level: `notice`.
Used as the default and is used for non-warning messages the user should
nevertheless be aware of. (#2027)
* config:
* New config parser (#1377)
* `include-directory` key (#2196), see #1946
* Add error message when an entire section is missing. (#2195)
* `-minlen` and `-alignment` properties for labels. (#1546)
* Make the `seperator` key in the bar section a label. (#1918)
* Better color validation. (#1290)
* timer modules: Schedule module updates to be aligned with the update interval.
For example, the date module now updates on the minute instead of in the
middle of a minute if the interval is set to 60 seconds. (#2123), see #2064
* `custom/menu`: Multiple menu modules per bar (#1907)
* `internal/backlight`: Support for changing the brightness through scrolling.
This may require additional changes to the system running polybar. (#1957)
* `internal/github`:
* `format-offline` for when the module cannot connect to the server (#1825),
see #1815
* Support for github enterprise instances. (#1841), see #1812
* `internal/network`: Support `Gbit/s` for `%linkspeed%` token. (#2055)
* `internal/pulseaudio`:
* `click-right` and `click-middle` keys (#1941)
* `%decibels%` token (#1894), see #1886
* `internal/xworkspaces`: Proper implementation for `label-occupied`. (#822),
see #874, #1444, #1033
**Fixes**
* Polybar not executing commands that produce output. (#1680), see #916
* Polybar froze until click commands finished executing (#2248)
* Polybar not properly working with mirrored monitors. (#1823), see #1192 and
#1794
* Unstable animation framerate (#1683), see #568
* Multiple modules of the same type caused click events not to be delivered to
the rigth one (#1907), see #1172
* config:
* Seemingly unrelated error messages when BOM character appears in config.
(#2166), see #2075
* Fall back to next possible config location if config file doesn't exist
(except if `--config` was used). (#2026), see #2016
* iconset: `fuzzy-match` chose first match, even if exact match was available.
(#2042), see #2041
* `custom/menu`: Spacing issue (#1656)
* `internal/alsa`: Volume didn't go over 100% (#2184), see #2173
* `internal/backlight`: Use amdgpu workaround for all devices starting with
`amdgpu_bl`. (#2122)
* `internal/battery`: Battery not marked as full if over `full-at` percent.
(#2019), see #1622
* `internal/cpu`: More accurate cpu load calculation. (#1955)
* `internal/github`: Outdated GitHub API authentication. (#2029), see #2002
* `internal/memory`: Use the correct size prefixes (#2211), see #2023
* `internal/network`:
* Wrong up- and downspeed values for non-integer intervals. (#2219)
* tun/tap interfaces didn't query their IP addresses. (#1990), see #1986
* Don't crash module if linkspeed cannot be queried. (#1772), see #1211
* `internal/temperature`:
* `format-warn` was not used if the temperature was *exactly*
`warn-temperature`. (#1897)
* `internal/xworkspaces`:
* Multi-monitor issue (#1929), see #1764, #1849
* Module sometimes showed too many workspaces (#1984), see #1983
* build:
* xpp submodule doesn't work with python 3.9 (polybar/xpp#26)
* CMake 3.17+ developer warnings (#2089, polybar/xpp#24, polybar/xpp#25)
* gtest compilation failure (#1993), see google/googletest#2678
* Compilation issue in GCC 6. (#1953)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Release Note:
Once this is part of a release we can do the following:
This PR changes the behavior of the
prevandnextactions in the i3 and bspwm module (formerlyi3wm-wsnext,i3wm-wsprev,bspwm-desknext, andbspwm-deskprev).The old behavior of those actions was affected by the
reverse-scrollsetting. This behavior is very counterintuitive and it makes more sense thatnextandprevselect the next and previous workspace respectively.reverse-scrollis applied when selecting the correct action for the two scroll directions./Release Note
This is my first draft to make input handlers more robust, make input action differentiable from actions that execute commands and map input action to a single module.
For now I left the oldinput_handlerinterface and used the dynamic pointer cast. I don't really know what is considered best practice here but I'm not really fond of it.I also considered just dropping
input_handlerand making all modules input handlers.What do you think @Lomadriel?
EDIT: I integrated the input_handler into the module_interface class. Now only modules are input handlers.
I now need to also adapt every single module that is an input handler as well as create an exception for the
menu-*,i3wm-*, andbspwm-*actions since they are part of the public API and can only be removed in the next major version.Fixes #1172
EDIT:
The built documentation for this branch can be viewed here
TODO:
exec-Naction to menu module so we know when to close the menu (With this we could easily implement Don't close menu on click #845). Theexec-Naction should produce a new action (not just run the shell command). This prevents code duplication for running shell commands and also allows menus to execute internal polybar actions (as they currently can as well). With this a user could trigger an infinite loop of actions by settingmenu-0-0-exec = #menu.exec.0-0#name.actionas the format. Looks a bit nicermenu-open-1orbspwm-deskfocus0+4). Maybe we could use a format like#name.action[.data]for this.