Fix bar subcommand handler structs and selection #2808

RedSoxFan · 2018-10-09T17:44:04Z

Overview of changes:

Made it so mode and hidden_state are valid both in the config and during runtime.
Made it so id and swaybar_command are only valid in the config.
Moves the config-only subcommand handling to below bar selection/creation.
Only allows creation of custom id bars during runtime (default id bars in config only)

For a follow-up PR:
Once #2751 is merged, send the IPC barconfig_update event for the subcommands in cmd_bar_handlers and handle it in swaybar

RedSoxFan · 2018-10-09T17:45:45Z

@ianyfan This should fix #2791 (comment) and the runtime prevention in #2751

RyanDwyer · 2018-10-09T22:23:03Z

Now that load_swaybar exists and is public, the status_command can be changed to call it only for the bar being changed rather than reloading all of them using load_swaybars.

ianyfan · 2018-10-10T12:15:20Z

Bug - this config is invalid:

bar {
    id position
}

RedSoxFan · 2018-10-10T14:54:22Z

Bug - this config is invalid: bar id position

Good catch. Should be fixed now.

ianyfan · 2018-10-10T18:24:46Z

So what is the planned syntax for performing runtime commands for specific bars? Are you going to modify every command so that it can accept bar <subcommand> [<bar_id>]?

RedSoxFan · 2018-10-10T18:33:58Z

So what is the planned syntax for performing runtime commands for specific bars?

The easiest would be just bar <bar-id> <subcommand> since that already sets config->current_bar. For i3 compatibility, bar mode|hidden_state [<bar-id>] should also be allowed though.

ianyfan · 2018-10-10T18:37:21Z

Would there be any issues with bars having an id that is the same as the name of a subcommand? i.e. any ambiguities

ianyfan · 2018-10-10T20:38:22Z

Also what would happen if the command was bar <id> mode ... <different_id>?

RedSoxFan · 2018-10-11T02:51:54Z

Would there be any issues with bars having an id that is the same as the name of a subcommand? i.e. any ambiguities

With the last commit, the only invalid id should be id. It also makes it so subcommands cannot be the first argument of the subcommand (with the except of <bar> id <subcommand-as-id>), but unless I'm missing something, I don't think that'll be a problem. I'm also not sure how many people would use any of the current subcommands as an id anyway.

Also what would happen if the command was bar <id> mode ... <different_id>?

It could go one of three ways: (1) use <id>, (2) use <different_id>, or (3) be invalid. My vote is for (2), where <different_id> has preference when given, <id> is second preferred, and all bars is the fallback (for i3 compat).

ianyfan

I still need to review the later logic, but I'm not sure if it would change depending on these changes.

sway/commands/bar.c

ianyfan · 2018-10-12T18:25:13Z

sway/commands/bar.c

 	if (argc > 1) {
 		struct bar_config *bar = NULL;
-		if (!find_handler(argv[0], bar_handlers, sizeof(bar_handlers))
-				&& find_handler(argv[1], bar_handlers, sizeof(bar_handlers))) {
+		if (!is_subcommand(argv[0]) ||


What if we disallowed bar <id> <subcommand> in the config file?
Would this allow this check to be changed to !config->reading && find_handler(argv[1], bar_handlers, sizeof(bar_handlers))? Then id id would not have to be disallowed.
I'm not sure if this would change some later logic.

If a user wanted to define multiple bars in the config, this would force bar blocks again (expanded would work fine for one bar). Since there is no release allowing the bar <bar-id> <subcommand> syntax, this could be an option since there probably aren't many people who have the expanded notation in their config.

Also, some insight into allowing bar <bar-id> <subcommand> was to make it uniform with input <input-name>, output <output-name>, mode <mode-name>, seat <seat-name>, etc.

Okay, it should be fine to keep the behaviour as it is.

However, is the first check !is_subcommand(argv[0]) actually required? Can't think of a (valid) case when that would be true but the later check false.

ianyfan · 2018-10-13T09:48:09Z

Also what would happen if the command was bar mode ... <different_id>?

It could go one of three ways: (1) use , (2) use <different_id>, or (3) be invalid. My vote is for (2), where <different_id> has preference when given, is second preferred, and all bars is the fallback (for i3 compat).

Actually, I think it should be the other way round, since if is new then it would spawn a new bar, and if the mode didn't affect that it would be weird. This would require small changes to the command code.

Or maybe both should be changed, I don't know what happens in i3 if you include mode ... <different-id> in a bar block the config file.

ianyfan · 2018-10-13T09:56:43Z

Why is id and swaybar_command not being allowed at runtime?

Ideally, this will be replaced with an IPC barconfig_update event in the near future

RedSoxFan · 2018-10-13T12:13:07Z

Actually, I think it should be the other way round, since if is new then it would spawn a new bar, and if the mode didn't affect that it would be weird. This would require small changes to the command code.

Sounds good to me. Order of preference should then be: (1) config->current_bar, (2) arg[2] in bar_cmd_mode, and finally(3) all bars

Why is id and swaybar_command not being allowed at runtime?

id because it would require a barrename IPC event since bars only listen for their bar-id for barconfig_update IPC events.

swaybar_command to be consistent with swaybg_command and swaynag_command in sway/commands.c

ianyfan · 2018-10-13T20:29:12Z

sway/commands/bar.c

 			}
+		}
+		if (!bar) {
+			spawn = !config->reading;


Would it be possible to change spawning behaviour so it only needs to allocate once?

I'm thinking something like using a variable like spawn_with_id.

I'm also confused why this loop is required later:

sway/sway/commands/bar.c

Line 95 in 782a835

for (int i = 0; i < config->bars->length; ++i) {

since wouldn't the bar always be the last one?

Anyway, I think this may be outside of the scope of this PR, and I may open my own one. It otherwise looks good to me, and if you don't feel like dealing with this I'm happy to accept it.

Would it be possible to change spawning behaviour so it only needs to allocate once? I'm thinking something like using a variable like spawn_with_id.

I'm not sure I understand what you are asking. That's line is inside the creation of a bar with a custom id. spawn would not be set to true if the bar already exists.

I'm also confused why this loop is required later [..] since wouldn't the bar always be the last one?

I'm actually not sure. I'm guessing its an artifact from an older version of the codebase. I'll remove the loop

The loop has been removed. I also fixed up a few of the commits to group them better since there were multiple small related changes.

Allows bar-subcommand to be a valid bar-ids Destroys runtime created bar if trying to use a config only subcommand Allow subcommands (except for id) to be ids

ianyfan

LGTM and works

ddevault · 2018-10-14T12:30:55Z

Thanks!

ianyfan · 2018-10-14T12:40:26Z

@RedSoxFan If you make the follow-up PR, can you remember to change the man pages to reflect this change in behaviour? Thanks

RedSoxFan force-pushed the bar-subcommands branch from 2b0ac2c to ac57935 Compare October 10, 2018 14:53

ddevault approved these changes Oct 10, 2018

View reviewed changes

RedSoxFan force-pushed the bar-subcommands branch from ac57935 to 6dc453f Compare October 11, 2018 02:42

ianyfan self-requested a review October 11, 2018 12:37

ianyfan requested changes Oct 12, 2018

View reviewed changes

RedSoxFan force-pushed the bar-subcommands branch from 6dc453f to 64b32d6 Compare October 13, 2018 01:54

ianyfan mentioned this pull request Oct 13, 2018

Bar mode/hidden_state events #2751

Merged

4 tasks

RedSoxFan added 2 commits October 13, 2018 08:00

Fix bar subcommand handler structs and selection

00745d6

bar_cmd_status_command: only reload current bar

2340b14

Ideally, this will be replaced with an IPC barconfig_update event in the near future

RedSoxFan force-pushed the bar-subcommands branch from 64b32d6 to e246a78 Compare October 13, 2018 12:14

ianyfan reviewed Oct 13, 2018

View reviewed changes

RedSoxFan added 2 commits October 13, 2018 17:55

cmd_bar: fix bar id issues

84b28dc

Allows bar-subcommand to be a valid bar-ids Destroys runtime created bar if trying to use a config only subcommand Allow subcommands (except for id) to be ids

cmd_bar: simplify logic

2a0c7eb

RedSoxFan force-pushed the bar-subcommands branch from e246a78 to 2a0c7eb Compare October 13, 2018 21:58

ianyfan approved these changes Oct 14, 2018

View reviewed changes

ddevault merged commit abde9d6 into swaywm:master Oct 14, 2018

RedSoxFan deleted the bar-subcommands branch October 14, 2018 15:02

RedSoxFan mentioned this pull request Mar 21, 2019

double swaybar when using colors #3946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bar subcommand handler structs and selection #2808

Fix bar subcommand handler structs and selection #2808

RedSoxFan commented Oct 9, 2018

RedSoxFan commented Oct 9, 2018

RyanDwyer commented Oct 9, 2018

ianyfan commented Oct 10, 2018

RedSoxFan commented Oct 10, 2018

ianyfan commented Oct 10, 2018

RedSoxFan commented Oct 10, 2018

ianyfan commented Oct 10, 2018 •

edited

Loading

ianyfan commented Oct 10, 2018

RedSoxFan commented Oct 11, 2018 •

edited

Loading

ianyfan left a comment

ianyfan Oct 12, 2018 •

edited

Loading

RedSoxFan Oct 13, 2018 •

edited

Loading

ianyfan Oct 13, 2018

RedSoxFan Oct 13, 2018

ianyfan commented Oct 13, 2018

ianyfan commented Oct 13, 2018

RedSoxFan commented Oct 13, 2018

ianyfan Oct 13, 2018

RedSoxFan Oct 13, 2018

RedSoxFan Oct 13, 2018

ianyfan left a comment

ddevault commented Oct 14, 2018

ianyfan commented Oct 14, 2018

Fix bar subcommand handler structs and selection #2808

Fix bar subcommand handler structs and selection #2808

Conversation

RedSoxFan commented Oct 9, 2018

RedSoxFan commented Oct 9, 2018

RyanDwyer commented Oct 9, 2018

ianyfan commented Oct 10, 2018

RedSoxFan commented Oct 10, 2018

ianyfan commented Oct 10, 2018

RedSoxFan commented Oct 10, 2018

ianyfan commented Oct 10, 2018 • edited Loading

ianyfan commented Oct 10, 2018

RedSoxFan commented Oct 11, 2018 • edited Loading

ianyfan left a comment

Choose a reason for hiding this comment

ianyfan Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

RedSoxFan Oct 13, 2018 • edited Loading

Choose a reason for hiding this comment

ianyfan Oct 13, 2018

Choose a reason for hiding this comment

RedSoxFan Oct 13, 2018

Choose a reason for hiding this comment

ianyfan commented Oct 13, 2018

ianyfan commented Oct 13, 2018

RedSoxFan commented Oct 13, 2018

ianyfan Oct 13, 2018

Choose a reason for hiding this comment

RedSoxFan Oct 13, 2018

Choose a reason for hiding this comment

RedSoxFan Oct 13, 2018

Choose a reason for hiding this comment

ianyfan left a comment

Choose a reason for hiding this comment

ddevault commented Oct 14, 2018

ianyfan commented Oct 14, 2018

ianyfan commented Oct 10, 2018 •

edited

Loading

RedSoxFan commented Oct 11, 2018 •

edited

Loading

ianyfan Oct 12, 2018 •

edited

Loading

RedSoxFan Oct 13, 2018 •

edited

Loading