Add option to remove desktop after focus change #3

MyNameIsCosmo · 2019-01-21T20:32:18Z

This will allow you to keep an empty desktop open if you're focused in it
Set remove-focused = false in your config.toml to active this feature

Fixes #1

This will allow you to keep an empty desktop open if you're focused in it Set `remove-focused = false` in your config.toml to active this feature

MyNameIsCosmo · 2019-01-21T20:33:44Z

I have only tested this on 1 monitor, so I am not sure of the behavior across monitors.

MyNameIsCosmo · 2019-01-25T03:33:59Z

config/config.go

@@ -89,6 +90,7 @@ func newDefaultConfig() *viper.Viper {
 	c.SetDefault("min", 1)
 	c.SetDefault("max", math.MaxInt64)
 	c.SetDefault("remove-empty", true)
+	c.SetDefault("remove-focused", true)


@cmschuetz Should btops default to not removing focused desktops by default (false) or should it keep the previous behavior of removing focused desktops (true)?

I would prefer to keep it in line with previous behavior, so I think true here works.

With that said, remove-focused is sort of tied to remove-empty as it shouldn't have any effect if remove-empty is false and so this naming might be a bit confusing taken out of context

Was thinking about this a bit and figured we could merge this config with remove-empty but instead provide a distinct option for this case.

E.g. possible options: remove-empty: [true, unfocused, false]

We'd have to change this config from a bool to a string but I think this strikes a nice balance without adding more top-level configs and would not be a breaking change.

Long term I was thinking we could possibly supply a white-list of applications where this rule applies. E.g. remove all empty, focused desktops except when the last closed program was steam. This is something I would personally want as it's annoying to open steam while it brings up and tears down several windows. However, this would require a substantial code change as we'd need a log of previous bspwm states and I think we can solve this problem in other ways, will create issues for some more ideas

That makes sense, and I agree (looking back now) that remove-focused is confusing.

Perhaps desktop pinning would be a solution to your second note?
I.E. allow a way to "pin" a desktop from being removed even remove-empty = true.

For a whitelist, maybe a configurable timeout before removing could be written? For example, if Steam opens on a desktop, that desktop will have a timeout of say 60s before it is removed if it's empty. This can rely on a check added to the main loop.

MyNameIsCosmo · 2019-01-25T03:48:31Z

handlers/handlers.go

 				continue
 			}

-			if r.config.Min >= len(monitor.Desktops) {
+			// TODO: Should we handle desktop destruction if the monitor focus is switched?
+			if !r.config.RemoveFocused && monitor.FocusedDesktopId == desktop.Id {


@cmschuetz One side effect of this is that an empty desktop could end up in the middle of the desktops.
I.E. where bold means not empty
"1 2 3 4"
// close window in 2, 2 is focused so btops removes 4
"1 2 3"

I suggest adding in a MoveHandler, and having a config option move-empty where a value such as first, last, keep would move the empty desktop to the beginning, end, or not move the desktop (respectively)
A MoveHandler may eventually be used along with the RenameHandler to move a desktop to the specified position if it contains a window or has a name, i.e:

[[names.classified]] "web" = ["Chromium", "Firefox"] position = 0 [[names.classified]] "" = ["Termite", "Alacritty", "URxvt"] position = 1

This would share a similar priority metric as RenameHandler would (if I'm skimming the code correctly) in addition to a check if the position is available (i.e. if there's only 1 desktop, no movement operations are needed, or if the position requested is 5, but there's only 3 desktops, move the desktop to the end)
One unexpected side effect from the second example would be the case where there are 4 desktops, a new one is created, and the empty desktop is switched with the position 5 desktop. Another option could be added to swap with empty desktops...

I think we accomplish the desired outcome with this line as-is without the need for adding a new handler. Essentially, we want this function to take no action if remove-focused == false && currently focused desktop is empty. Once we focus a new desktop, bspwm will send us another message and your TODO comment is handled automatically. I was able to reach what I believe is your desired outcome with:

func (r RemoveHandler) Handle(m *monitors.Monitors) bool { for _, monitor := range *m { for _, desktop := range monitor.EmptyDesktops() { if *desktop == monitor.Desktops[len(monitor.Desktops)-1] { continue } if !r.config.RemoveFocused && monitor.FocusedDesktopId == desktop.Id { continue } if r.config.Min >= len(monitor.Desktops) { continue } err := monitor.RemoveDesktop(desktop.Id) if err != nil { log.Println("Unable to remove desktop: ", desktop.Name, err) continue } return true } } return false }

With the above, if desktop 2 is empty, neither 2 nor 4 are removed. Once I switch to another desktop, 2 is cleaned up as desired. Is there another case i'm not considering here?

This seems like it would do it.
I wanted to wait on some feedback on the expected behavior on empty desktop removal priority (or expectation?) before continuing with more logic.

cmschuetz

This is looking solid so far, let me know what you think of these ideas and we'll hopefully get this through shortly. Thanks for taking action here

cmschuetz · 2019-06-09T14:22:57Z

config/config.go

@@ -89,6 +90,7 @@ func newDefaultConfig() *viper.Viper {
 	c.SetDefault("min", 1)
 	c.SetDefault("max", math.MaxInt64)
 	c.SetDefault("remove-empty", true)
+	c.SetDefault("remove-focused", true)


I would prefer to keep it in line with previous behavior, so I think true here works.

With that said, remove-focused is sort of tied to remove-empty as it shouldn't have any effect if remove-empty is false and so this naming might be a bit confusing taken out of context

Was thinking about this a bit and figured we could merge this config with remove-empty but instead provide a distinct option for this case.

E.g. possible options: remove-empty: [true, unfocused, false]

We'd have to change this config from a bool to a string but I think this strikes a nice balance without adding more top-level configs and would not be a breaking change.

Long term I was thinking we could possibly supply a white-list of applications where this rule applies. E.g. remove all empty, focused desktops except when the last closed program was steam. This is something I would personally want as it's annoying to open steam while it brings up and tears down several windows. However, this would require a substantial code change as we'd need a log of previous bspwm states and I think we can solve this problem in other ways, will create issues for some more ideas

cmschuetz · 2019-06-09T14:25:10Z

handlers/handlers.go

@@ -134,12 +134,16 @@ func (r RemoveHandler) ShouldHandle() bool {

 func (r RemoveHandler) Handle(m *monitors.Monitors) bool {
 	for _, monitor := range *m {
+        if len(monitor.EmptyDesktops()) == 1 {
+            return true
+        }


Returning true signifies that we've made changes to bspwm we should wait to receive the next message before making any more modifications, thus short-circuiting the handler flow. This will cause all downstream handlers to not get reached

Ah, I see that now.

Side note: I will fix formatting in the next commit

cmschuetz · 2019-06-09T14:29:53Z

handlers/handlers.go

 		for _, desktop := range monitor.EmptyDesktops() {
-			if *desktop == monitor.Desktops[len(monitor.Desktops)-1] {


What's the reason for taking this check out? This prevents removing the last desktop if it's empty

I don't remember the reason, but can test when I spin up a vm with bspwm later this week.

I vaguely remember that this line did not respect the minimum desktop configuration, and would end up removing more desktops than the user has configured (if they have a minimum of 3 desktops, this may remove that third desktop)

cmschuetz · 2019-06-09T14:38:24Z

handlers/handlers.go

 				continue
 			}

-			if r.config.Min >= len(monitor.Desktops) {
+			// TODO: Should we handle desktop destruction if the monitor focus is switched?
+			if !r.config.RemoveFocused && monitor.FocusedDesktopId == desktop.Id {


I think we accomplish the desired outcome with this line as-is without the need for adding a new handler. Essentially, we want this function to take no action if remove-focused == false && currently focused desktop is empty. Once we focus a new desktop, bspwm will send us another message and your TODO comment is handled automatically. I was able to reach what I believe is your desired outcome with:

func (r RemoveHandler) Handle(m *monitors.Monitors) bool { for _, monitor := range *m { for _, desktop := range monitor.EmptyDesktops() { if *desktop == monitor.Desktops[len(monitor.Desktops)-1] { continue } if !r.config.RemoveFocused && monitor.FocusedDesktopId == desktop.Id { continue } if r.config.Min >= len(monitor.Desktops) { continue } err := monitor.RemoveDesktop(desktop.Id) if err != nil { log.Println("Unable to remove desktop: ", desktop.Name, err) continue } return true } } return false }

With the above, if desktop 2 is empty, neither 2 nor 4 are removed. Once I switch to another desktop, 2 is cleaned up as desired. Is there another case i'm not considering here?

CumpsD · 2019-07-30T09:59:14Z

Looking forward to this :)

MyNameIsCosmo · 2019-07-30T18:52:56Z

@cmschuetz - I just saw your notes, thanks for the feedback!

I switched to using Sway since I've been migrating towards a Wayland-oriented setup.
If I have some free time I'll take another pass addressing the feedback.

Add option to remove desktop after focus change

e028a07

This will allow you to keep an empty desktop open if you're focused in it Set `remove-focused = false` in your config.toml to active this feature

MyNameIsCosmo mentioned this pull request Jan 21, 2019

Don't immediately switch off empty desktop #1

Open

MyNameIsCosmo added 2 commits January 24, 2019 21:54

Add remove-focused to readme

b48916b

Only remove a desktop if more than one is empty

fea79f4

MyNameIsCosmo commented Jan 25, 2019

View reviewed changes

cmschuetz reviewed Jun 9, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to remove desktop after focus change #3

Add option to remove desktop after focus change #3

MyNameIsCosmo commented Jan 21, 2019

MyNameIsCosmo commented Jan 21, 2019

MyNameIsCosmo Jan 25, 2019

cmschuetz Jun 9, 2019

MyNameIsCosmo Jul 30, 2019

MyNameIsCosmo Jan 25, 2019

cmschuetz Jun 9, 2019

MyNameIsCosmo Jul 30, 2019

cmschuetz left a comment

cmschuetz Jun 9, 2019

cmschuetz Jun 9, 2019

MyNameIsCosmo Jul 30, 2019

cmschuetz Jun 9, 2019

MyNameIsCosmo Jul 30, 2019

cmschuetz Jun 9, 2019

CumpsD commented Jul 30, 2019

MyNameIsCosmo commented Jul 30, 2019

		for _, desktop := range monitor.EmptyDesktops() {
		if *desktop == monitor.Desktops[len(monitor.Desktops)-1] {

Add option to remove desktop after focus change #3

Are you sure you want to change the base?

Add option to remove desktop after focus change #3

Conversation

MyNameIsCosmo commented Jan 21, 2019

MyNameIsCosmo commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmschuetz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CumpsD commented Jul 30, 2019

MyNameIsCosmo commented Jul 30, 2019