feat(github): offline label & fixes #1825

kopecs · 2019-06-25T03:42:41Z

Should resolve the segfault issue with #910. The defaults are such that if offline the module displays Offline instead of the current Notifications: -1. Notifications are still set to -1 and a flag is set to indicate online/offline status which causes format-offline to be used when offline.

I haven't tested exhaustively but everything seemed to be working with a couple different configs on my machine.

MAINTAINER EDIT:
Closes #910

Lomadriel

You just need to remove some bugs and I think it'll be okay.
I'll just wait the opinion of @patrick96 to know if we choose to rename the default formatter & label to format-online and label-online.

src/modules/github.cpp

include/modules/github.hpp

patrick96 · 2019-06-25T22:53:34Z

Thanks for reviewing @Lomadriel. I think we should stay with format and label since those represent the default state. Having "Offline" as the default should be fine. Though mpd actually has a default of "" for format-offline, so you only need to set label-offline when you set format-offline, I think that is counter intuitive so this approach here seems better since it gives sane defaults.

Lomadriel · 2019-06-26T02:32:15Z

src/modules/github.cpp

+      m_label_offline = load_optional_label(m_conf, name(), TAG_LABEL_OFFLINE, "Offline");
    }
+
+    update_label(0);


I'm not sure if we should keep this line.
Indeed it prevents to display the label without applying the data but this isn't done in any module.
Maybe, we should address this globally rather that adding quick fixes in every modules.

As that was already the case, perhaps this isn't the best pr to address changing it in? Other than that the issues you noted should be fixed.

I think we can let this line for now. When I'll post a PR that solves these issues I'll remove it.

Lomadriel

LGTM.

Lomadriel · 2019-06-28T02:22:38Z

src/modules/github.cpp

    try {
      content = request();
    } catch (application_error& e) {
      m_log.warn("%s: cannot complete the request to github: %s", name(), e.what());


I didn't think about that before, but since "offline" is now a normal state, maybe we should replace this line by:

m_log.info("%s: cannot complete the request to github: %s", name(), e.what());

What do you think of that @patrick96?

Good idea, we don't want to spam users with warnings that aren't really warnings.

kopecs · 2019-07-07T23:16:48Z

Any more steps we need to take to get this merged?

Lomadriel · 2019-07-19T16:06:16Z

Any more steps we need to take to get this merged?

This PR will be merged soon but the core team is a little busy.
I think your PR will be integrated into the 3.5.0 since we have already started to package the 3.4.0.

kopecs · 2019-07-19T17:44:30Z

Sounds good! Thanks for the update!

Lomadriel · 2019-08-04T19:10:07Z

Hello, one of my PR (#1841) just get merged and it introduced a conflict with yours.
Can I ask you to resolve it?

kopecs · 2019-08-04T21:27:50Z

Can you confirm that change resolves any conflict? It looks like we just both added a new member in the same place.

Lomadriel · 2019-08-04T21:39:44Z

Looks fine, that was the only conflict.
Thanks !

patrick96

Code looks good. But I am still getting a segfault with the following config:

[module/gh]
type = internal/github
token = XXXXXXX
interval = 10
format = GH

I think it happens because you are missing a NULL check for a label somewhere.

src/modules/github.cpp

kopecs · 2020-01-21T21:09:50Z

Best I can tell the segfault occurs as a result of attempting to clear the label in update_label when the format property is of certain (invalid?) format, e.g., text without any labels. Is there documentation on the grammar of the format property besides the description here? I'm not totally sure what the resolution here should be; if m_label is a nullptr should I just immediately return from update_label with a warning, or is another solution preferred?

As an additional note, I'm assuming any build issues currently present are a result of google/googletest#2678.

patrick96 · 2020-01-21T23:48:50Z

Oh that segfault isn't even a result of your PR, but was there from the beginning. In other modules we always first check if a label is valid before dereferencing it, for example in the cpu module:

polybar/src/modules/cpu.cpp

Lines 69 to 78 in 4ef2bd5

    
           if (m_label) { 
        
             m_label->reset_tokens(); 
        
             m_label->replace_token("%percentage%", to_string(static_cast<int>(m_total + 0.5))); 
        
             m_label->replace_token("%percentage-sum%", to_string(static_cast<int>(m_total * static_cast<float>(cores_n) + 0.5))); 
        
             m_label->replace_token("%percentage-cores%", string_util::join(percentage_cores, "% ") + "%"); 
        
             for (size_t i = 0; i < percentage_cores.size(); i++) { 
        
               m_label->replace_token("%percentage-core" + to_string(i + 1) + "%", percentage_cores[i]); 
        
             } 
        
           }

We shouldn't print a warning since not having a label is completely valid (though not really useful)

As far as I can tell you only need to update update_label. However, this isn't really a bug in your PR, so we can also just merge this as-is.

We don't really have any developer documentation on the formats. But a format can contain an arbitrary string. Occurences of certain label, ramp, bar, animation names inside angle brackets (<, >) are treated as referencing the contents of that label, ramp etc. So format = foo is just as valid as format = foo <label> bar or just format = <label>.

As an additional note, I'm assuming any build issues currently present are a result of google/googletest#2678.

Yes, the test build fails because of that. It's fixed on master.

kopecs · 2020-01-22T03:06:15Z

@patrick96 I've made the requested change about the logging level. I also added checks to confirm there aren't any nullptr dereferences (although if you'd prefer those are fixed separately I can roll it back). Is there any more action to be take at the moment?

patrick96

That was fast 😃

I'm wondering why you now also check clear and reset the offline label in the update_label method. Is there anything I am not seeing? Because I would have thought checking if m_label is null and directly returning at the start of update_label would be enough.

kopecs · 2020-01-27T16:53:18Z

Yeah, the most recent revision should be sufficient. I just looked at it again today and I can't think of any reason I'd need to do that and it seems fine without.

Because I would have thought checking if m_label is null and directly returning at the start of update_label would be enough.

In this case wouldn't I still need to check m_offline_label, or if one is non-null are they both set? I'm not totally clear on what holds with the label settings.

patrick96 · 2020-01-27T18:43:42Z

In this case wouldn't I still need to check m_offline_label

You only need to check m_offline_label before you dereference it. And there is no need to call reset_tokens on m_offline_label because it doesn't have any tokens. So you never derference it. You pass it to the builder though, but there the builder does its own checks.

EDIT: The two labels are completely independent. They are null if you don't assign them in your module (there is no outside influence or any magic black box here), in this case here they are not assigned, if they do not appear in any of the formats (which is possible).

kopecs · 2020-02-09T21:48:24Z

The most recent commit shouldn't have any NULL dereferences and doesn't clear tokens for things that don't have them.

patrick96

Sorry, last nitpick, afterwards we can merge this ;)

src/modules/github.cpp

kopecs · 2020-02-14T04:34:41Z

No problem! Nit picks appreciated.

patrick96

Looks good now!

Thank you! 🚀

patrick96

Oh no, there is a merge conflict and github doesn't even know why 😞

kopecs · 2020-02-18T20:30:48Z

Let me know if there's still an issue with merging; I pulled master into this branch and it seemed to resolve it fine but I hadn't and don't see anywhere on github where it's indicating issues with merging to me (modulo that fact that I have changes requested).

codecov · 2020-02-18T20:38:19Z

Codecov Report

Merging #1825 into master will increase coverage by 0.08%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #1825      +/-   ##
=========================================
+ Coverage    9.21%    9.3%   +0.08%     
=========================================
  Files         180     180              
  Lines       10923   10823     -100     
=========================================
  Hits         1007    1007              
+ Misses       9916    9816     -100

Flag	Coverage Δ
#unittests	`9.3% <0%> (+0.08%)`	⬆️

Impacted Files	Coverage Δ
include/modules/github.hpp	`0% <ø> (ø)`	⬆️
src/modules/github.cpp	`0% <0%> (ø)`	⬆️
include/components/config.hpp	`0% <0%> (ø)`	⬆️
include/x11/xresources.hpp	`0% <0%> (ø)`	⬆️
include/events/signal.hpp	`0% <0%> (ø)`	⬆️
include/adapters/alsa/generic.hpp	`0% <0%> (ø)`	⬆️
include/x11/connection.hpp	`0% <0%> (ø)`	⬆️
include/x11/winspec.hpp	`0% <0%> (ø)`	⬆️
include/adapters/net.hpp	`0% <0%> (ø)`	⬆️
include/components/types.hpp	`0% <0%> (ø)`	⬆️
... and 2 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 8d2b0d2...253f6c1. Read the comment docs.

patrick96

Yeah, github couldn't even tell me why it couldn't merge it. Quite a journey this one.

Now it seems merging the master branch has downgraded the xpp module. I have quickly fixed this myself.

patrick96 · 2020-02-21T09:11:19Z

This is starting to make me mad. Now there is a merge conflict again 😠

…alse

patrick96 · 2020-02-21T12:48:41Z

Not even cherry-picking the individual commits in this PR worked. I have now taken the diff of this entire PR and just applied it to our master branch and just force pushed. I have also noticed that we missed something: When empty-notifications is false, the label is supposed to be empty if there are no notifications, but this PR removed the call to clear, if have also fixed this.

EDIT: Since I force-pushed, you won't be able to pull from this branch. I'd recommend adding this repo as a new remote named upstream and run:

git fetch upstream
git reset --hard upstream/master

This will discard any changes in this branch and make it up-to-date with master.

kopecs · 2020-02-21T16:50:43Z

Sorry that this was such a headache to get done. Thanks for the update!

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)

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)

Lomadriel requested changes Jun 25, 2019

View reviewed changes

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

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

Lomadriel reviewed Jun 26, 2019

View reviewed changes

Lomadriel approved these changes Jun 28, 2019

View reviewed changes

Lomadriel reviewed Jun 28, 2019

View reviewed changes

Lomadriel added this to the 3.5.0 milestone Jul 19, 2019

Lomadriel mentioned this pull request Jul 28, 2019

Support for GitHub Enterprise in github module #1812

Closed

patrick96 requested changes Jan 21, 2020

View reviewed changes

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

patrick96 reviewed Jan 23, 2020

View reviewed changes

patrick96 requested changes Feb 13, 2020

View reviewed changes

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

patrick96 approved these changes Feb 16, 2020

View reviewed changes

patrick96 requested changes Feb 16, 2020

View reviewed changes

patrick96 approved these changes Feb 21, 2020

View reviewed changes

feat(github): offline label & fixes

2656457

patrick96 added 2 commits February 21, 2020 13:42

Clear label if there are no notifications and empty-notifications = f…

2da2108

…alse

clang-format

253f6c1

patrick96 approved these changes Feb 21, 2020

View reviewed changes

patrick96 merged commit 683cfc0 into polybar:master Feb 21, 2020

patrick96 mentioned this pull request Nov 29, 2020

Release 3.5.0 #2253

Merged

28 tasks

Uh oh!

feat(github): offline label & fixes #1825

feat(github): offline label & fixes #1825

Uh oh!

Conversation

kopecs commented Jun 25, 2019 • edited by patrick96 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lomadriel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

patrick96 commented Jun 25, 2019

Uh oh!

Lomadriel Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

kopecs Jun 28, 2019

Choose a reason for hiding this comment

Uh oh!

Lomadriel Jun 28, 2019

Choose a reason for hiding this comment

Uh oh!

Lomadriel left a comment

Choose a reason for hiding this comment

Uh oh!

Lomadriel Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrick96 Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

kopecs commented Jul 7, 2019

Uh oh!

Lomadriel commented Jul 19, 2019

Uh oh!

kopecs commented Jul 19, 2019

Uh oh!

Lomadriel commented Aug 4, 2019

Uh oh!

kopecs commented Aug 4, 2019

Uh oh!

Lomadriel commented Aug 4, 2019

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kopecs commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick96 commented Jan 21, 2020

Uh oh!

kopecs commented Jan 22, 2020

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

kopecs commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrick96 commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kopecs commented Feb 9, 2020

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kopecs commented Feb 14, 2020

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

patrick96 left a comment

Choose a reason for hiding this comment

Uh oh!

kopecs commented Jun 25, 2019 •

edited by patrick96

Loading

Lomadriel Jun 28, 2019 •

edited

Loading

kopecs commented Jan 21, 2020 •

edited

Loading

kopecs commented Jan 27, 2020 •

edited

Loading

patrick96 commented Jan 27, 2020 •

edited

Loading

codecov bot commented Feb 18, 2020 •

edited

Loading

patrick96 commented Feb 21, 2020 •

edited

Loading

patrick96 commented Feb 21, 2020 •

edited

Loading