search

tobi-wan-kenobi / bumblebee-status

bumblebee-status is a modular, theme-able status line generator for the i3 window manager.
https://bumblebee-status.readthedocs.io/en/main/
MIT License
1.2k stars 228 forks source link

name attribute of an i3bar block is not unique #561

Closed ghost closed 4 years ago

ghost commented 4 years ago

Bug Report

subj

Summary

Affected module: output.py

Description:

I had a look at the output of bumblebee-status (don't ask me why, but for now I'll just say that the reason was a bit more than just hypothetical) and I believe I spotted a place where it is not compliant to the i3bar protocol specification.

Quote from https://i3wm.org/docs/i3bar-protocol.html :

name and instance

Every block should have a unique name (string) entry so that it can be easily identified in scripts which process the output. i3bar completely ignores the name and instance fields. Make sure to also specify an instance (string) entry where appropriate. For example, the user can have multiple disk space blocks for multiple mount points.

First of all, there's no instance attribute anywhere (in master branch, but there is in development branch). I'm not sure this part is a problem at all for the master branch, while bumblebee-status can use a generic module like spacer, the naming uniqueness is achieved through aliases.

The problem with the name attribute seems to be a real one. For example, a module with multiple widgets (think mpd) for example, will produce multiple blocks with same "mpd" name. Indeed, I can see

"name": module.id,

in output.py. And development branch seems to be suffering from same defect, it uses 

"name": module.id(),

So every mpd related block for every prefix/suffix/full_text part of each mpd widget is named "mpd".

This is very misleading for exactly the reason stated in the i3bar docs that I quoted above. Can we please get unique names like MODULENAME.WIDGETNAME/ID.PREFIX/FULL_TEXT/SUFFIX everywhere (in both master and development branches)? If possible, in master branch first.

tobi-wan-kenobi commented 4 years ago

Hi,

instance should be set in output.py:464. My interpretation of the protocol spec was that the name is the "type" of the block (disk, in the given example), and the instance is used to disambiguate, and this is what bumblebee does, both in master and in development.

This is the only implementation I can think of where it makes sense to have both a name and an instance, otherwise, the information would just be redundant, if I am not overlooking something.

Using the instance alone should be sufficient if uniqueness is key, because those are just uuids (if this didn't work, click events would be broken, they use the instance internally, I hope that helps! If there's any additional problem I am not seeing right now, please let me know!

On Feb 24, 2020, 18:55, at 18:55, somospocos notifications@github.com wrote:

Bug Report

subj

Summary

Affected module: output.py

Description:

I had a look at the output of bumblebee-status (don't ask me why, but for now I'll just say that the reason was a bit more than just hypothetical) and I believe I spotted a place where it is not compliant to the i3bar protocol specification.

Quote from https://i3wm.org/docs/i3bar-protocol.html :

name and instance

Every block should have a unique name (string) entry so that it can be easily identified in scripts which process the output. i3bar completely ignores the name and instance fields. Make sure to also specify an instance (string) entry where appropriate. For example, the user can have multiple disk space blocks for multiple mount points.

First of all, there's no instance attribute anywhere (in master branch, but there is in development branch). I'm not sure this part is a problem at all for the master branch, while bumblebee-status can use a generic module like spacer, the naming uniqueness is achieved through aliases.

The problem with the name attribute seems to be a real one. For example, a module with multiple widgets (think mpd) for example, will produce multiple blocks with same "mpd" name. Indeed, I can see

"name": module.id,

in output.py. And development branch seems to be suffering from same defect, it uses

"name": module.id(),

So every mpd related block for every prefix/suffix/full_text part of each mpd widget is named "mpd".

This is very misleading for exactly the reason stated in the i3bar docs that I quoted above. Can we please get unique names like MODULENAME.WIDGETNAME/ID.PREFIX/FULL_TEXT/SUFFIX everywhere (in both master and development branches)? If possible, in master branch first.

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561

ghost commented 4 years ago

My mistake, somehow I missed the instance attribute in the json output (I guess I should've formatted it better before looking at it). But it's still just an unique id that is machine friendly rather than human friendly.

I'll try to reword just in case if the original post looks like an X-Y problem.

Let's say I want to parse the json output generated by bumblebee-status. For simplicity, let's consider that only mpd module is enabled. It generates multiple blocks.

In context of a single block, how can I answer the following questions:

  1. Which widget of the mpd module generated this block?

  2. Is this a prefix block? A full_text block? A suffix block of that particular widget?

All blocks are named "mpd" and those unique instance values do not contain such info.

tobi-wan-kenobi commented 4 years ago

Ah, I see. Yes, that is not possible right now. The IDs were mostly targeted at reading click events.

547 introduced a kind of work around, in that each module now has a parameter "id" that is a comma-separated list of IDs, one per widget.

The core challenge is to find a generic solution that generates predictable IDs. Each of the approaches I can think of break as soon as multiple instances of bumblebee-status are running.

The other option I am considering is using custom i3 fields (_separator, for example) as metadata. Not sure how many usecases that'd cover, though.

⁣Sent from TypeApp ​

On Feb 24, 2020, 19:24, at 19:24, somospocos notifications@github.com wrote:

My mistake, somehow I missed the instance attribute in the json output (I guess I should've formatted it better before looking at it). But it's still just an unique id that is machine friendly rather than human friendly.

I'll try to reword just in case the original post looks like an X-Y problem.

Let's say I want to parse the json output generated by bumblebee-status. For simplicity, let's consider that only mpd module is enabled. It generates multiple blocks.

In context of a single block, how can I answer the following questions:

  1. Which widget of the mpd module generated this block?

  2. Is this a prefix block? A full_text block? A suffix block of that particular widget?

All blocks are named "mpd" and those unique instance values do not contain such info.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-590477690

ghost commented 4 years ago

The core challenge is to find a generic solution that generates predictable IDs.

What's wrong with paterns like MODULENAME.WIDGETNAME.full_text in name field?

Each of the approaches I can think of break as soon as multiple instances of bumblebee-status are running.

Since I don't know in detail about everything that's happening behind the curtain in the core, I can't comment on this because I simply don't understand it. I didn't think about this because my use case would be trying to parse data from a single bumblebee-status instance and only care about text, nothing event related. All I want is to be able to figure out where does a block belong (which module, which widget of that module and is it a prefix, full_text or suffix block for that widget) - this without leaving the context of this single isolated block (like not having to resort to counting blocks in the list and making judgements based on their index).

tobi-wan-kenobi commented 4 years ago

MODULENAME.WIDGETNAME.full_text breaks as soon as you use a module twice (though you can use aliases to work around that).

If you run bumblebee twice, there's no uniqueness, so "simulating" clicm events wouldn't work. Since that is an existing use case, I'd like to come up with a splution that supports it.

Separators "belong" to two widgets, not sure how to handle that.

Does the id parameter help you?

I'll think a bit more, many thanks for your iput!

On Feb 25, 2020, 03:58, at 03:58, somospocos notifications@github.com wrote:

The core challenge is to find a generic solution that generates predictable IDs.

What's wrong with paterns like MODULENAME.WIDGETNAME.full_text in name field?

Each of the approaches I can think of break as soon as multiple instances of bumblebee-status are running.

Since I don't know in detail about everything that's happening behind the curtain in the core, I can't comment on this because I simply don't understand it. I didn't think about this because my use case would be trying to parse data from a single bulbebee-status instance and only care about text, nothing event related. All I want is to be able to figure out where does a block belong (which module, which widget of that module and is it a prefix, full_text or suffix block for that widget) - this without leaving the context of this single isolated block (like not having to resort to counting blocks in the list and making judgements based on their index).

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-590657390

ghost commented 4 years ago

MODULENAME.WIDGETNAME.full_text breaks as soon as you use a module twice (though you can use aliases to work around that).

I feel like I slip here. Isn't the current pattern for the name attribute just MODULENAME? So for mpd module in the example above it's "mpd" everywhere. The part that I'm failing to understand is why

{ "name": "mpd" }

doesn't cause any problem currently (I didn't try to run the mpd module twice though, and if I had to use a spacer twice, I would indeed use aliases, to achieve naming uniqueness for the sake of ricing them correctly in themes), but

{"name": "mpd.prev.prefix"}

would cause problems.

Separators "belong" to two widgets, not sure how to handle that.

Uhh, that's a dark area for me as well, but so far I am not aiming to process separator blocks, since some themes can have no separator at all, as far as I know. But I agree I would need a way to tell that a block is a separator, so I can skip it, maybe here your "_separator" metadata field idea could work.

Does the id parameter help you?

Which id parameter? I am looking at the JSON output that a custom config of mine is generating and I don't see any "id" keys in block dictionaries. Is this something that I can explicitly specify as module parameter with -p modulename.id="foo"?

tobi-wan-kenobi commented 4 years ago

Ah, I think now I understand better.

Yes, you are right, changing the name to be more unique woild not break a lot (only the recently introduced bumblebee-cmd). I have to admit that I am rather fond of the current approach, though. It allows to select by type (via the name) as well as by actual instance (uuid, though that is problematic since the id is not predictable)

How about a) making "instance" more structured (e.g. a json string that includes both the id and the fully qualified name you suggested) b) add a custom field _fullname to the output that contains your suggestion?

Reg. the id parameter: In the latest git (master), you can specify -p mpd.id="a,b,c,d" to give the first 4 widgets if mpd the IDs (instance) a, b, c and d, respectively. Does that help your specific requirements?

On Feb 25, 2020, 06:13, at 06:13, somospocos notifications@github.com wrote:

MODULENAME.WIDGETNAME.full_text breaks as soon as you use a module twice (though you can use aliases to work around that).

I feel like I slip here. Isn't the current pattern for the name attribute just MODULENAME? So for mpd module in the example above it's "mpd" everywhere. The part that I'm failing to understand is why

{ "name": "mpd" }

doesn't cause any problem currently (I didn't try to run the mpd module twice though, and if I had to use a spacer twice, I would indeed use aliases, to achieve naming uniqueness for the sake of ricing them correctly in themes), but

{"name": "mpd.prev.prefix"}

would cause problems.

Separators "belong" to two widgets, not sure how to handle that.

Uhh, that's a dark area for me as well, but so far I am not aiming to process separator blocks, since some themes can have no separator at all, as far as I know. But I agree I would need a way to tell that a block is a separator, so I can skip it, maybe here your "_separator" metadata field idea could work.

Does the id parameter help you?

Which id parameter? I am looking at the JSON output that a custom config of mine is generating and I don't see any "id" keys in block dictionaries. Is this something that I can explicitly specify as module parameter with -p modulename.id="foo"?

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-590687600

ghost commented 4 years ago

Reg. the id parameter: In the latest git (master), you can specify -p mpd.id="a,b,c,d" to give the first 4 widgets if mpd the IDs (instance) a, b, c and d, respectively. Does that help your specific requirements?

I just used -p mpd.id="a,b,c,d" and looked at the produced JSON again. So this option that I didn't know before just overwrites the id keys in the block dictionary with custom values instead of those UUID. This is a game changer, since I can just explicitly overwrite those ids everywhere for all widgets of all modules when I'm invoking bumblebee-status from command line and not actually have it launched by i3 to make use of the event handling machinery. The only assumption I can make about a possible problem with this approach is for setups with modules that don't have a predefined fixed amount of widgets, but where the widgets are added based on external data. I think sensors2 does something like that? But this is not my use case so far.

I think I can try working with this and I'll see if it will be enough or if I'll get stuck and the codebase will actually require any of the changes that were considered above. Feel free to close the issue, I will bump it later if I'll encounter any obstacle in achieving what I'm planning to try.

ghost commented 4 years ago

Here is a minimal demo of bumblebee-status working with something else than i3

https://github.com/somospocos/bumblebee-bridge-dwm

tobi-wan-kenobi commented 4 years ago

That is very very cool, thanks! Never imagined that :-)

Also, great that the id override works for you. ⁣ ​

On Feb 25, 2020, 12:17, at 12:17, somospocos notifications@github.com wrote:

Here is a minimal demo of bumblebee-status working with something else than i3

https://github.com/somospocos/bumblebee-bridge-dwm

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-590817003

ghost commented 4 years ago

Also, great that the id override works for you.

This minimal prototype doesn't yet make use of the advanced id override feature yet. But I am planning to enhance the dwm bridge to support colors through a custom dwm patch written by a kind dwm hacker.

I am also planning to try writing a dzen2 bridge using the same idea. Because dzen2 supports mouse events, but the event handlers in bumblebee-status aren't passed in the JSON output, my idea is to detect specific blocks that are known to have such events (think mpd buttons or pulseaudio that can mute/change volume level) and insert custom events like shell commands to mimic the functionality of event handlers in bumblebee-status modules. This is where explicitly naming widgets should shine.

tobi-wan-kenobi commented 4 years ago

The functionality in bumblebee-cli migut be helpful for that, maybe?

Looking forward to this A LOT :-)

On Feb 25, 2020, 16:06, at 16:06, somospocos notifications@github.com wrote:

Also, great that the id override works for you.

This minimal prototype doesn't yet make use of the advanced id override feature yet. But I am planning to enhance the dwm bridge to support colors through a custom dwm patch written by a kind dwm hacker.

I am also planning to try writing a dzen2 bridge using the same idea. Because dzen2 supports mouse events, but the event handlers in bumblebee-status aren't passed in the JSON output, my idea is to detect specific blocks that are known to have such events (think mpd buttons or pulseaudio that can mute/change volume level) and insert custom events like shell commands to mimic the functionality of event handlers in bumblebee-status modules. This is where explicitly naming widgets should shine.

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-590912468

ghost commented 4 years ago

bumblebee-bridge-dwm now supports colors for custom patched dwm versions. I have updated the README with detailed instructions about how to get it working. I have also updated the README on https://github.com/somospocos/bumblebee-status-contrib and linked to the dwm bridge, so careful readers of README files can reach the bridge repo from bumblebee-status. But feel free to link it directly in the README of this repo. Frankly, I'm not too proud of the code quality, and it has no tests at all, but so far it works.

ghost commented 4 years ago

The dwm bridge has a brother now. It lives at https://github.com/somospocos/bumblebee-bridge-dzen2

It is already functional, but I am planning to try adding support for mouse events to this one, as well as support for transparently replacing prefixes with image icons, because dzen2 supports a couple of simple image formats.

ghost commented 4 years ago

While during the discussion in this issue its name became inaccurate (it started with my rant about the name attribute due to me not knowing the internals of bumblebee-status well enough, but it ended being about the instance attribute and about the -p MODULENAME.id argument that lets the user customize those identifiers), I would like to try pushing the madness I'm doing a bit further.

First of all, I would like to say that the suggestion with custom ids worked great for labeling widget blocks. You can see the result in the dzen2 bridge repository. The README is completed with instructions to get a dzen2 setup that still pulls visual data from bumblebee-status, yet it is also able to react to mouse events just like the i3 bar. The event handlers have to be redefined manually though, but it works.

But I've hit another problem now. I would like the dzen2 bridge to replace prefixes in the input it gets from bumblebee-status with icons, because dzen2 supports real icon files. The show stopper here is that I don't know how to detect those prefixes in the JSON output outside of bumblebee-status context. Consider these examples:

{
    "full_text": "<span foreground='#EBCB8B' >Prev</span>",
...

This is the "mpd.prev" widget of the mpd module in the JSON output. "Prev" is the prefix from my custom icon theme. The "full_text" attribute of the widget is empty string.

 {
    "full_text": "<span foreground='#EBCB8B' >></span>songname",
...

This is the "mpd.main" widget of the mpd module in the JSON output. ">" is the prefix for the "playing" status from my custom theme. The "full_text" attribute of the widget is not empty - it contains the song name. 

{
    "full_text": "<span foreground='#EBCB8B' >></span>\u0420\u043e\u043a-\u041e\u0441\u0442\u0440\u043e\u0432\u0430 - \u041c\u043e\u0435 \u0434\u0432\u0438\u0436\u0435\u043d\u0438\u0435 2:1",
...

This is the "cpu2.loads" widget of the cpu2 module. The first <span> block is the prefix of this widget and it's empty - because I set it to "" in my icon theme.

These examples show that the structure of "full_text" attribute in these JSON blocks is not uniform:

My trouble - from the perspective of finding prefixes in this JSON output - is that prefixes belong to the same "full_text" attributes of the same i3bar blocks as the "full_text" attributes of widgets. You can see that the "full_text" concept is giving me a small headache here, because it's overloaded and can mean two different things.

It would be easier for me if prefixes had their own blocks with a metadata

"_prefix": true

attribute for example.

But:

  1. Such a change would require a massive overhaul in the drawing machinery

  2. I don't know how nice would it play with keeping backwards compatibility - an event handler is supposed to work for a whole widget, so from this perspective having one JSON i3bar block per widget is correct, for both bumblebee-status interacting with i3bar and for bumblebee-status interacting with dzen2.

ghost commented 4 years ago

It would be easier for me if prefixes had their own blocks with a metadata

"_prefix": true

attribute for example.

I gave it some more thought and I think changing the output of bumblebee-status this much is probably overkill.

I think the current structure of i3bar JSON blocks should stay as it is.

Time for some fact checking.

  1. Technically, a "prefix" (in context of theme) is a piece of text that gets prepended to "full_text" attribute of a widget.

  2. There is just one prefix in the full_text field of an i3bar JSON block and, obviously, it appears at the beginning of that string.

  3. In context of the full_text field of a JSON i3bar block, the prefix string isn't just the string that appears in the used theme. It can be wrapped in pango markup (either when the theme contains fg/bg custom settings for prefixes, or via --iconmarkup).

If all of the above is true, perhaps it would be possible to record the size of this whole prefix string in a metadata _prefixlength field when it gets added to full_text? This way I will know exactly how much should I cut off from the beginning. Combined with labeling widgets with custom ids via MODULENAME.id parameters, hypothetically, it should provide the necessary toolset for me:

tobi-wan-kenobi commented 4 years ago

After thinking about it for some time: Wouldn't it be easiest if I gave you the "raw" input data (without decorations such as the prefix, suffix and padding)?

Because I guess similarily to the prefix being "real" icons, you'd have the same issue with the suffix (e.g. for the power supply).

I'll implement that experimentally and let you judge :)

ghost commented 4 years ago

I had a look at the JSON output and compared full_text and _raw fields. Am I understanding it right that full_text = prefix + _raw + suffix? So technically full_text.split(_raw) == [prefix, suffix]? And if either (or both) prefix, suffix are missing, their value will just be '' (empty string) in the result list produced by split()?

tobi-wan-kenobi commented 4 years ago

Yes, that should be the case, exactly.

On Feb 29, 2020, 14:21, at 14:21, somospocos notifications@github.com wrote:

I had a look at the JSON output and compared full_text and _raw fields. Am I understanding it right that full_text = prefix + _raw + suffix? So technically full_text.split(_raw) == [prefix, suffix]? And if either (or both) prefix, suffix are missing, their value will just be '' (empty string) in the result list produced by split()?

-- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-592944967

ghost commented 4 years ago

Fine, I will try working with this then. Thank you.

ghost commented 4 years ago

I have just realized there is an edge case. It is possible for _raw to be equal to empty string. This can happen, for example, when one uses an aliased spacer widget with empty text - just for the sake of displaying icons. Like any widget, it can have either a prefix or a suffix in the icon theme. Or even both. _raw being empty, it means I can't use it to split the whole full_text string. So I have a full_text string. I know that _raw is empty, but somehow I need to figure out if full_text is just the prefix, just the suffix, or both. And if it's both - where does the prefix end and suffix start.

It looks like an additional metadata field might be needed - the length of prefix, like I suggested above. Then:

tobi-wan-kenobi commented 4 years ago

What if I add the prefix and the suffix string separately as metadata? Not sure why exactly, but the length feels like it could limit use cases in the future.

On Feb 29, 2020, 20:33, at 20:33, somospocos notifications@github.com wrote:

I have just realized there is an edge case. It is possible for _raw to be equal to empty string. This can happen, for example, when one uses an aliased spacer widget with empty text - just for the sake of displaying icons. Like any widget, it can have either a prefix or a suffix in the icon theme. Or even both. _raw being empty, it means I can't use it to split the whole full_text string. So I have a full_text string. I know that _raw is empty, but somehow I need to figure out if full_text is just the prefix, just the suffix, or both. And if it's both - where does the prefix end and suffix start.

It looks like an additional metadata field might be needed - the length of prefix, like I suggested above. Then:

  • if _raw == "" and len(prefix) == len(full_text) - I'll know that full_text contains just the prefix
  • if _raw == "" and len(prefix) == 0, I'll know that full_text contais just the suffix
  • if _raw == "" and 0 < len(prefix) < len(full_text), I'll know both prefix and suffix are present and I'll split full_text

-- You are receiving this because you modified the open/close state. Reply to this email directly or view it on GitHub: https://github.com/tobi-wan-kenobi/bumblebee-status/issues/561#issuecomment-592986089

ghost commented 4 years ago

I don't mind that, if it's not a problem for data redundancy to become too big.

Technically, _prefix + _raw + _suffix == full_text, so a block would contain the same data twice. This is why initially I was only thinking about having offsets only as metadata.

There can be a metadata list with 3 items, -1 meaning missing

Examples:

len(full_text) = 30 for all cases

So passing _prefix and _suffix as metadata will increase the size of each block but will make processing faster, since I will be able to just pick the parts directly instead of computing them from indexes. If you don't think this will cause a performance problem (it probably really shouldn't, because the amounts of text are quite small anyway), I don't mind having _prefix and _suffix metadata parts.

Edit: last second thought: it is possible to avoid the redundancy. bumblebee-status would just need to get one more command line boolean argument and depending on its value include these metadata fields or not. There is really no need to include data twice if it will go into i3bar. But I can toggle the --metadata switch on for my bridge use case.

ghost commented 4 years ago

My apologies for typing before finishing the thought. Actually, redundance can be avoided entirely.

  1. revert the addition of _raw attribute
  2. add a --metadata argument to bumblebee-status
  3. if missing, bumblebee-status will keep working as it is
  4. if true, JSON blocks will contain _prefix, _raw, _suffix fields INSTEAD of full_text, since _prefix + _raw + _suffix == full_text anyway.
tobi-wan-kenobi commented 4 years ago

The "--metadata" is a good idea. For now, I will leave it out, as the protocol is just a stdin/stdout protocol anyhow, so I think efficiency isn't too important (i.e. verbosity doesn't hurt a lot).

For the same reason, I'm keeping all 3 fields for now, no telling who might need which pieces in the future :)

ghost commented 4 years ago

Icon support for the dzen2 bridge has been added and documented.

Thanks for your help @tobi-wan-kenobi