Xkeyboard use variant by dvermd · Pull Request #2521 · polybar/polybar

dvermd · 2021-10-03T07:03:29Z

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

Description

The new configuration can use variant.

; layout-icon-[0-9]+ = layout;icon
; layout-icon-[0-9]+ = layout;variant;icon
; Assign each layout an icon that will be available as %icon% token for the
; <label-layout> tag. 
; In the first configuration form, the `layout` will try to match %layout% value without using any variant
; In the second configuration form, 
;     the `layout` will try to match %layout% value and the `variant` will try to match %variant%.
;     the `variant` can be empty to match against a %layout% with an empty %variant%
;     the `variant` can be the wildcard '_' to match a %layout% and any variant. 
;         This is equivalent to the first configuration form
;     the `layout` can be the wildcard '_' to match any %layout% with a specific %variant%
;     if both the `layout` and the `variant` are '_', the layout-icon-default it overwritten
; The ';variant;icon' is an invalid configuration because there can't be a %variant% without a %layout% and a warning is logged
; The ';icon' and ';_;icon' are  invalid configuration because they define a default icon and layout-icon-default must be used for that. A configuration error is issued and the module is not loaded.

Related Issues & Documents

Closes #2414

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

dvermd · 2021-10-03T07:23:31Z

Part of the configuration I used for validating warning and errors:

[bar/bar_test_issue]
monitor = ${env:MONITOR:HDMI-1}
width = 100%
height = 18

font-0 = fixed:pixelsize=10;1

modules-center = i3 xkeyboard_test_issue xkb_test2 xkb_test3 xkb_test4 xkb_test5 xkb_test6 xkb_test7
scroll-up = #i3.prev
scroll-down = #i3.next

;==========================================================
[module/xkb_test_issue]
type = internal/xkeyboard
blacklist-0 = scroll lock

label-layout = %layout%-%icon%-%variant%

layout-icon-0 = us;U
layout-icon-1 = us;colemak;U2  

;==========================================================
[module/xkb_test2]
type = internal/xkeyboard
blacklist-0 = num lock
label-layout = 2-%layout%-%icon%
layout-icon-0 = us;U
layout-icon-1 = us;_;U2

;==========================================================
[module/xkb_test3]
type = internal/xkeyboard
blacklist-0 = num lock
label-layout = 3-%layout%-%icon%
layout-icon-0 = us;U
layout-icon-1 = us;U2

;==========================================================
[module/xkb_test4]
type = internal/xkeyboard
blacklist-0 = num lock
label-layout = 4-%layout%-%icon%
layout-icon-0 = us;colemak;U
layout-icon-1 = us;colemak;U2

;==========================================================
[module/xkb_test5]
type = internal/xkeyboard
blacklist-0 = num lock
label-layout = 5-%layout%-%icon%
layout-icon-default = DEF
layout-icon-0 = _;_;U

;==========================================================
[module/xkb_test6]
type = internal/xkeyboard
blacklist-0 = num lock
label-layout = 6-%layout%-%icon%
layout-icon-default = DEF
layout-icon-0 = _;U

;==========================================================
[module/xkb_test7]
type = internal/xkeyboard
blacklist-0 = num lock
label-layout = 7-%layout%-%icon%
layout-icon-0 = malformed value
layout-icon-1 = too;many;items;to;unpack
layout-icon-2 = ;_;icon

Edit: indicator-icon-default -> layout-icon-default

codecov · 2021-10-03T07:35:07Z

Codecov Report

Merging #2521 (5d43ba1) into master (8afd5b7) will increase coverage by 0.20%.
The diff coverage is 30.23%.

❗ Current head 5d43ba1 differs from pull request most recent head aa461d5. Consider uploading reports for the commit aa461d5 to get more accurate results

@@            Coverage Diff             @@
##           master    #2521      +/-   ##
==========================================
+ Coverage   10.24%   10.44%   +0.20%     
==========================================
  Files         145      147       +2     
  Lines       10096    10155      +59     
==========================================
+ Hits         1034     1061      +27     
- Misses       9062     9094      +32

Flag	Coverage Δ
unittests	`10.44% <30.23%> (+0.20%)`	⬆️

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

Impacted Files	Coverage Δ
include/components/config.hpp	`0.00% <0.00%> (ø)`
include/components/controller.hpp	`0.00% <0.00%> (ø)`
include/components/eventloop.hpp	`0.00% <ø> (ø)`
include/modules/ipc.hpp	`0.00% <ø> (ø)`
include/modules/menu.hpp	`0.00% <ø> (ø)`
include/modules/meta/base.hpp	`0.00% <ø> (ø)`
include/modules/meta/base.inl	`0.00% <0.00%> (ø)`
include/modules/meta/static_module.hpp	`0.00% <0.00%> (ø)`
include/modules/xworkspaces.hpp	`0.00% <ø> (ø)`
include/tags/action_context.hpp	`100.00% <ø> (ø)`
... and 37 more

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 8afd5b7...aa461d5. Read the comment docs.

patrick96

I did some reorganizing in parse_icons and applied clang-format.

The logic seems sound thought the layouticonset.get function is quite complicated and there are some redundancies. I added some comments for that.

include/drawtypes/layouticonset.hpp

patrick96 · 2021-10-03T19:17:41Z

include/utils/string.hpp

  using hash_type = unsigned long;

  bool contains(const string& haystack, const string& needle);
+  bool contains_nocase(const string& haystack, const string& needle);


Maybe rename to contains_ignore_case

src/drawtypes/layouticonset.cpp

src/modules/xkeyboard.cpp

include/utils/string.hpp

src/modules/xkeyboard.cpp

dvermd · 2021-10-04T05:09:45Z

Once reviewed, some commits may be merged to keep a nice history

dvermd · 2021-10-04T20:34:37Z

src/modules/xkeyboard.cpp

-        variant = vec[1];
-        icon = vec[2];
-      }
+      const string& variant = size == 2 ? layouticonset::VARIANT_ANY : vec[1];


I think there might be a problem with this solution because VARIANT_ANY is a static constexpr const char* and the compiler will need to create a string from it to assign this const reference but this string may be created on the stack and the reference may not live long enough to be used correctly once the ternary operator finishes.
I may be wrong as I'm not fluent in c++ const_expr

I don't think that is an issue here. The way I understand it is that from VARIANT_ANY a string object is created, a reference to which is stored in variant. The lifetime of this object is prolonged to match the lifetime of the reference.

patrick96 · 2021-10-04T21:35:13Z

I rewrote the best-match function because it bothered me that we have so many flags that we have to check. I also slightly changed the semantics that once we have a match of a certain priority, we can't have another match of the same priority. In practice this means if we have multiple case-insensitive matches, the first one is used.

patrick96

Please also add an entry in the changelog under the Added section.

* feat(string_util): add contains_nocase * feat(xkeyboard): Enable icon by variant * Cleanup * string_util: add some cases to string test * string_util: rename contains_nocase -> contains_ignore_case * layouticonset: use contains_ignore_case * layouticonset: apply renamings and remove dead code * remove VARIANT_NONE and use empty string instead * use emplace_back and add assert * layouticonset: precompute condition * xkeyboard: restore missing continue * Cleanup parse_icons * Always choose the first case-insensitive match * Cleanup layouticonset.get * update the changelog Co-authored-by: patrick96 <[email protected]>

dvermd added 2 commits September 29, 2021 22:49

feat(string_util): add contains_nocase

f8a3e6d

feat(xkeyboard): Enable icon by variant

a45b404

Cleanup

d916f7c

patrick96 requested changes Oct 3, 2021

View reviewed changes

dvermd commented Oct 4, 2021

View reviewed changes

src/modules/xkeyboard.cpp Outdated Show resolved Hide resolved

dvermd commented Oct 4, 2021

View reviewed changes

include/utils/string.hpp Show resolved Hide resolved

dvermd commented Oct 4, 2021

View reviewed changes

src/modules/xkeyboard.cpp Show resolved Hide resolved

dvermd added 7 commits October 4, 2021 07:04

string_util: add some cases to string test

71b91e1

string_util: rename contains_nocase -> contains_ignore_case

d4729df

layouticonset: use contains_ignore_case

a75a62c

layouticonset: apply renamings and remove dead code

93f2dac

remove VARIANT_NONE and use empty string instead

4d816be

use emplace_back and add assert

fda23d8

layouticonset: precompute condition

e57ec1c

dvermd and others added 2 commits October 4, 2021 21:41

xkeyboard: restore missing continue

37432ac

Cleanup parse_icons

9d2e577

dvermd commented Oct 4, 2021

View reviewed changes

patrick96 added 2 commits October 4, 2021 23:02

Always choose the first case-insensitive match

65c9c00

Cleanup layouticonset.get

5f7506a

patrick96 requested changes Oct 4, 2021

View reviewed changes

update the changelog

aa461d5

patrick96 approved these changes Oct 5, 2021

View reviewed changes

patrick96 merged commit 98dffc2 into polybar:master Oct 5, 2021

Uh oh!

Conversation

dvermd commented Oct 3, 2021

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

Description

Related Issues & Documents

Documentation (check all applicable)

Uh oh!

dvermd commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 3, 2021 • 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!

patrick96 Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

dvermd Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvermd commented Oct 4, 2021

Uh oh!

dvermd Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

patrick96 Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

patrick96 commented Oct 4, 2021

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dvermd commented Oct 3, 2021 •

edited

Loading

codecov bot commented Oct 3, 2021 •

edited

Loading