Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement proper handling of output_format in py3status for i3bar, dzen2, xmobar, lemonbar, tmux, term, none #2104

Merged
merged 15 commits into from
Jun 18, 2023

Conversation

oaken-source
Copy link
Contributor

This PR makes py3status compatible with tmux, by implementing the status bar format expected by tmux in addition to the i3 / sway formats already present in py3status.

To enable the tmux format, I extended the domain of the --wm cli parameter to accept 'tmux' as argument.

py3status currently has problems running when directly spawned by tmux non-interactively in tmux.conf, and will cause the CPU usage to spike. this can be prevented by using script like follows:

set -g status-right '#(script -qfec "py3status --wm tmux -c ~/.config/py3status/config-tmux")'

@ultrabug
Copy link
Owner

Hi @oaken-source

Thanks for this interesting proposal. As discussed on IRC I'd rather use the output_format which sound more appropriate than diverging from the "wm" terminology.

Would you care doing this instead?

@oaken-source
Copy link
Contributor Author

apologies, it seems I completely misunderstood what you were asking on IRC. I agree that adapting the output_format parameter in the config file makes a lot more sense.

@oaken-source oaken-source force-pushed the feature/tmux-status-format branch from 47c5d9f to 95b2050 Compare February 27, 2022 18:34
@oaken-source
Copy link
Contributor Author

rebased to master and applied requested changes. this feature probably also needs a documentation change, but I'm unsure where the right spot would be to add it

@oaken-source oaken-source changed the title add tmux output format in addition to i3 / sway using the --wm switch add tmux output format in addition to i3 / sway using the output_format config option Feb 27, 2022
@ultrabug
Copy link
Owner

please check CI results, mostly black formatting

@ultrabug ultrabug self-assigned this Feb 28, 2022
@ultrabug ultrabug added the enhancement 😍 I have created an enhancement label Feb 28, 2022
py3status/core.py Outdated Show resolved Hide resolved
@oaken-source
Copy link
Contributor Author

please check CI results, mostly black formatting

I fixed the issues reported by tox in 7702cf1 -- thanks for pointing that out.

@oaken-source oaken-source force-pushed the feature/tmux-status-format branch 2 times, most recently from f6b1519 to 6d12ba6 Compare March 3, 2022 02:47
py3status/constants.py Outdated Show resolved Hide resolved
py3status/core.py Outdated Show resolved Hide resolved
# tmux allows configurable separators between bar entries.
# handle configured value or fall back to the matching default for output_format
self.output_format_separator = self.config["py3_config"]["py3status"].get(
"output_format_separator", DEFAULT_SEPARATORS[self.output_format]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be others like dzen2. https://i3wm.org/docs/i3status.html#_general
Edge cases to be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the link. this hints at the larger question whether py3status should support all values of output_format that i3status supports, although it's not immediately obvious to me how i3status integrates into py3status when the output_format is not i3bar. In my testing, I have so far actually not used any i3stauts modules, only pure py3status.

It might make sense to retire this PR for now, and carefully review the interaction between i3status and py3status with regards to different values of output_status before proceeding here.

Also, I see that i3status has a separator config option that covers the use case for which this PR introduced output_format_separator. Another good reason to review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial testing suggests that in fact the i3status integration of py3status will not work when output_format is anything else than "i3bar". setting a value of "tmux" will cause i3status to crash and py3status to emit a warning, setting a value of "term" will cause py3status to hang in setup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not immediately obvious to me how i3status integrates into py3status...

Since the inception, py3status was a python wrapper that could read i3status config and still be enhanced more with python (be it a modules or whatever).

Nowadays, I think it could be easier to break up i3status into its own module similar to i3block, i3pystatus and maybe conky... meaning for every i3status module you run, the process get spawned instead of currently one process for as many i3status modules as you want to add.

Big task to tackle on.... as we're not 100% there with i3status, but is more than enough for most users... and we would have to mimic any new or missing i3status features too.

py3status/core.py Outdated Show resolved Hide resolved
py3status/core.py Outdated Show resolved Hide resolved
@oaken-source
Copy link
Contributor Author

I took a step back, and started looking into i3status first. I created a PR for tmux output in i3status here:
i3/i3status#484

@oaken-source
Copy link
Contributor Author

Independently of the PR on i3status, it seems to me like the most reasonable way forward would be for py3status to require the value of output_format in general to be i3bar for compatibility with i3status, and introduce another value of output_format in the py3status section of the configuration. alternatively, it would be possible to generate a copy of the user-provided configuration that is passed to i3status, and modify the value of output_format in that config such that all i3status ever sees when called from py3status is a value of i3bar for output_format.

The first way would be easier to implement (add a second output_format setting for py3status) but the second way seems more user-friendly to me (providing i3status with an ephemeral, modified copy of the config file)

@oaken-source oaken-source changed the title add tmux output format in addition to i3 / sway using the output_format config option implement proper handling of output_format in py3status for i3bar, dzen2, xmobar, lemonbar, tmux, term, none Mar 6, 2022
@oaken-source
Copy link
Contributor Author

oaken-source commented Mar 6, 2022

this PR now enables full support for the all output_format values supported by i3status (plus tmux) in py3status.

py3status now carries an array of output_format values for which a join by separator is needed before printing the status line. this array is OUTPUT_FORMAT_NEEDS_SEPARATOR and is contained in constants.py. Also, there is a new array in constants.py that contains the default separator for values of output_format that do not default to the string " | ". (currently only dzen2)

This change required that the value of output_format that is passed to the i3status subprocess to be fixed to the value i3bar. This change is made when the temporary config file for the i3status process is created. This allows py3status to correctly parse the output of i3status regardless of the output_format setting. Output formatting is then performed by py3status afterwards.

otherwise, the behaviour of the output formatting is now identical to the i3status output, except for two edge cases:

1.) in the configuration file, a setting of output_format = none will cause i3status to use the none output formatting, while py3status will interpret the value as unset, and default to i3bar instead. to use the none output formatting in py3status, a setting of output_format = "none" is required (needs to be quoted).

2.) i3status uses the separator config setting in the general section to set the separator string between module outputs. Setting this in py3status causes an issue because the config expects separator to be a boolean when looking transitively through the configure options for the modules. Apparently, there is a difference here between the way py3status and i3status handle the configuration files. Note the HACK comment in py3status/module.py.

Copy link
Owner

@ultrabug ultrabug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for the time and energy you're putting into this.

Overall feeling is that maybe we could have made those outputs objects which all expose the same methods so each "implementation" would handle its own specificity

That would avoid large if / if / if conditions and would arguably make the code simpler to read.

That's just biased opinion tho.

@@ -10,6 +10,20 @@
"output_format": "i3bar",
}

OUTPUT_FORMAT_NEEDS_SEPARATOR = [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sort that list alphabetically

err += f"Got `{separator}`."
raise TypeError(err)
self.i3bar_module_options["separator"] = separator
# HACK: separator is a valid setting in the general section
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting, I did not recall that at all...

Copy link
Contributor

@lasers lasers Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick glance.. Maybe things are a bit hazy, but it looks like there could be two separator configs.

... both may gotten mixed... Putting separator = False in py3status config section should turn them all off.

Maybe we need two different separator coding.... One for general (string) and one for py3status (boolean).

EDIT: Or the separator = value was in wrong config section.... Need more inspecting.

@oaken-source
Copy link
Contributor Author

I agree with the point that these output formatters should probably be object oriented. this was just a quick way to write them very similarly to what is included in i3status, for ease of implementation and a first proof of concept. I'll prepare a new version where this is better encapsulated :)

@oaken-source
Copy link
Contributor Author

the latest commit introduces an object oriented approach to the output formatting. it's a lot more code than the if cascades, but probably better decoupled.

py3status/core.py Show resolved Hide resolved
# Set output_format to i3bar for parsing regardless of what
# formatting we apply ourselves before printing
if key == "output_format":
value = "i3bar"
Copy link
Contributor

@lasers lasers Apr 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeeeesh, fix this. See modules/khal_calendar.py

EDIT: Hmm. Not a i3status module, so it probably doesn't matter, but if one i3status module have it... Still yeeeesh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code actually fixes a long standing bug in py3status, where setting output_format to anything other than i3bar causes the i3status integration to hang indefinitely or crash.

py3status starts an i3status process internally, and parses its output to support i3status modules. This parsing requires the output to be the i3bar-compatible json format. if i3status produces any other output, which it will when output_format is set to anything other than i3bar, then the integration will break.

As a consequence, to make the output_format setting meaningful for py3status, it is necessary to overwrite the configured value in the generated config file before passing it to i3status.

Copy link
Contributor

@lasers lasers Apr 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that. I was initially reacting to modules having output_format config... which khal_calendar does have one, but then again, this doesn't write py3status modules in the config... so we're good here... then I wonder if it could be improved to only update the config inside general {} section instead. Either way, it's a good bug catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, I totally missed that this bit of code is run for all the i3status module sections as well. You're right, that should be fixed.

@oaken-source oaken-source force-pushed the feature/tmux-status-format branch from d5e1d16 to a3f619a Compare May 3, 2022 08:35
@ultrabug
Copy link
Owner

ultrabug commented Oct 2, 2022

@oaken-source getting back to this, I'd like to get it on py3status 3.48

Can you close all conversations you think have been addressed? I tried this PR on my machine and it works fine on i3bar format.

@lasers
Copy link
Contributor

lasers commented May 31, 2023

@ultrabug IIRC this looks 99% fine. Can address them later if necessary.

@ultrabug
Copy link
Owner

@lasers did you try it yourself? I did a long time ago... maybe I should give it a new try indeed

@lasers
Copy link
Contributor

lasers commented May 31, 2023

Not very recently. I looked at this weeks after this came out. LGTM. I have a diff after this that makes py3status act more like i3status in terminal. You can test this PR for few days too.

@oaken-source
Copy link
Contributor Author

fwiw, I'm still using it, but I haven't merged the master in a long while. There is only one unresolved issue, I think, and that's the one above about the output_format parameter being overwritten on the module level, which is not great. but I haven't found a way to fix that yet.

@lasers
Copy link
Contributor

lasers commented May 31, 2023

that's the one above about the output_format parameter being overwritten on the module level

IIRC that's for i3status modules only. AFAIK no i3status module have this config and only one py3status module does (khal_calendar) , but not applicable here so I think it's mostly an non-issue that could be looked again later only when it did happen.

@lasers
Copy link
Contributor

lasers commented Jun 2, 2023

@oaken-source This does it?

diff --git a/py3status/i3status.py b/py3status/i3status.py
index 8fc97e5..c3f6de4 100644
--- a/py3status/i3status.py
+++ b/py3status/i3status.py
@@ -330,10 +330,10 @@ class I3status(Thread):
                         value = TZTIME_FORMAT
                     if key == "format_time":
                         continue
-                # Set output_format to i3bar for parsing regardless of what
-                # formatting we apply ourselves before printing
-                if key == "output_format":
-                    value = "i3bar"
+                # force i3bar output_format in general
+                if section_name == "general":
+                    if key == "output_format":
+                        value = "i3bar"
                 if isinstance(value, bool):
                     value = f"{value}".lower()
                 self.write_in_tmpfile(f'    {key} = "{value}"\n', tmpfile)

@oaken-source oaken-source force-pushed the feature/tmux-status-format branch from a3f619a to 1da6c18 Compare June 6, 2023 07:58
@oaken-source
Copy link
Contributor Author

I rebased to master and applied the change that @lasers suggested above, that should do the trick. Apologies that I didn't get around to this sooner.

Tests look good of course, and I've been running the new build (albeit rebased to 3.50 not master) for a day without issues.

@ultrabug ultrabug merged commit dc3c2c4 into ultrabug:master Jun 18, 2023
@ultrabug
Copy link
Owner

Let's move on and take a leap of faith, thanks a lot for this great work @oaken-source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 😍 I have created an enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants