Skip to content

Add option to remove desktop after focus change #3

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.

Sign up for GitHub

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

Open
MyNameIsCosmo wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add option to remove desktop after focus change #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e028a07
Add option to remove desktop after focus change
MyNameIsCosmo Jan 21, 2019
b48916b
Add remove-focused to readme
MyNameIsCosmo Jan 25, 2019
fea79f4
Only remove a desktop if more than one is empty
MyNameIsCosmo Jan 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ Below are the different configuration options available. Please look at [exampl
| min | Int | Minimum number of desktops per monitor | 1 |
| max | Int | Maximum number of desktops per monitor | infinity |
| remove-empty | Bool | Removes empty desktops | true |
| remove-focused | Bool | Removes focused desktops | false |
| append-when-occupied | Bool | Appends a new desktop when all other desktops are occupied | true |
| watch-config | Bool | Reload btops on next event when configuration changes | true |
| renamers | []String | Order of [renamers](#renamers) to use for renaming desktops. If a given renamer is unable to rename a desktop, it cascades to the next renmaer | ["numeric"]
Expand Down
2 changes: 2 additions & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type Config struct {
Min int
Max int
RemoveEmpty bool `mapstructure:"remove-empty"`
RemoveFocused bool `mapstructure:"remove-focused"`
AppendWhenOccupied bool `mapstructure:"append-when-occupied"`
WatchConfig bool `mapstructure:"watch-config"`
configChangeC chan bool
Expand Down Expand Up @@ -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)
Copy link
Author

@MyNameIsCosmo MyNameIsCosmo Jan 25, 2019

Choose a reason for hiding this comment

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

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

Copy link
Owner

@cmschuetz cmschuetz Jun 9, 2019

Choose a reason for hiding this comment

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

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

Copy link
Author

@MyNameIsCosmo MyNameIsCosmo Jul 30, 2019

Choose a reason for hiding this comment

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

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.

c.SetDefault("append-when-occupied", true)
c.SetDefault("renamers", []string{numeric})
c.SetDefault("watch-config", true)
Expand Down
8 changes: 6 additions & 2 deletions handlers/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Copy link
Owner

@cmschuetz cmschuetz Jun 9, 2019

Choose a reason for hiding this comment

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

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

Copy link
Author

@MyNameIsCosmo MyNameIsCosmo Jul 30, 2019

Choose a reason for hiding this comment

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

Ah, I see that now.

Side note: I will fix formatting in the next commit

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

@cmschuetz cmschuetz Jun 9, 2019

Choose a reason for hiding this comment

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

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

Copy link
Author

@MyNameIsCosmo MyNameIsCosmo Jul 30, 2019

Choose a reason for hiding this comment

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

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)

if r.config.Min >= len(monitor.Desktops) {
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 {
Copy link
Author

@MyNameIsCosmo MyNameIsCosmo Jan 25, 2019

Choose a reason for hiding this comment

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

@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...

Copy link
Owner

@cmschuetz cmschuetz Jun 9, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Author

@MyNameIsCosmo MyNameIsCosmo Jul 30, 2019

Choose a reason for hiding this comment

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

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.

continue
}

Expand Down
1 change: 1 addition & 0 deletions monitors/monitors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type bspwmState struct {
type Monitor struct {
Name string
Id int
FocusedDesktopId int
Desktops []Desktop
}

Expand Down