wmutils / contrib

Useful bits and pieces
Other
94 stars 16 forks source link

focus.sh changes border to incorrect size #21

Closed brandonpaiz closed 8 years ago

brandonpaiz commented 8 years ago

Running focus.sh changes the window's border size to 2, which is not what it was/should be.

Changing line 4 to BW=$(wattr b $(pfw)) is my quick fix.

z3bra commented 8 years ago

That's an interresting approach. Using the current border to set the next one. But if you decide to put the current window fullscreen (so borderwidth=0), and then run focus.sh, it will set your next window's border to 0, and so on...

I choose to set the border width in focus.sh so all windows will have the same, no matter what. And this value defaults to 2 pixels.

If you want to set a different border width, you can "override" the default value of 2 by setting the $BW variable in your environment:

$ export BW=4
$ focus.sh next

Or setting it explicitely for each call to focus.sh:

$ BW=6 focus.sh prev 
brandonpaiz commented 8 years ago

Heh, "interesting" is a nice way of putting it.

Hopefully my new method is a little better.

z3bra commented 8 years ago

I don't think this method is better, for a few reasons:

0. Makes the overall look incoherent and runtime specific

The problem with this is that you'll loose the ability to have a "common" place to set the border. In a managed X environment, all windows should likely have the same border, and the easiest way to to this is by setting a default value that will be applied everywhere. I can't think of a use case where I'd want all windows to have a different border width (aside from a fullscreen window, but this issue is already addressed).

Moreover, this scripts are not supposed to be final solutions, but rather, a starting point for your own scripts. The goal is not to make the script work for everyone's use case, but demonstrate the most simple solution that would work, so that people could tweak it for their specific use.

1. Unnecessary complexity is added to fullscreen.sh

Your patch require fullscreen.sh to reset the border itself, disregarding the value focus.sh would set, or what is stored in the environment.

It makes you store this in a file, which is not a reliable method, because if you decide to change the overall border width, you will ALSO have to check this file, and replace the border value too, which is really complex for no particular reason.

By relying on focus.sh to reset the window's border, we use a "common interface" for this job.

2. More like a meta problem

If you propose a new and totally different solution, don't use an old pull request please. Especially if it include such bad commit messages...

brandonpaiz commented 8 years ago

This may not be a better method, but let me clear up some miscommunications.

In a managed X environment, all windows should likely have the same border, and the easiest way to to this is by setting a default value that will be applied everywhere.

I completely agreee. That is why, if $BW is set, focus.sh will use it. If the user neglected to do this, why change border width to some arbitrary number? Again, because all windows should likely have the same border, not changing the border when $BW is not set has a higher chance of success. Users can still customize border width by exporting this value, but now there is a safer fallback case. The border width that the user was already using is probably closer to what they want then a border width of two.

Your patch require fullscreen.sh to reset the border itself, disregarding the value focus.sh would set, or what is stored in the environment.

Not completely true. While fullscreen.sh does reset the border, it only resets it to the value it was before made fullscreen. Focus.sh is still called at the end of the fullscreen.sh script, so if the user has specified a $BW, it will come into effect. If not, the border will keep its original border width.

Focus.sh is still the common interface. Users can still

$ export BW=4
$ focus.sh next

or

$ BW=6 focus.sh prev

to specify border width.

About fullscreen.sh

It makes you store this in a file, which is not a reliable method, because if you decide to change the overall border width, you will ALSO have to check this file, and replace the border value too, which is really complex for no particular reason.

Just to reiterate, $BW will still be accounted for when focus.sh is called at the end of the fullscreen.sh script. I can't think of a situation in which you would have to check the file or change the value, but let me know if you have something in mind. Also, is storing border width in a preexisting file that stores information for the same window really all that unreliable?

I think the changes are better summerized like this

  1. fullscreen.sh will make windows fullscreen and restore them to their previous state, regardless of what variables have been set.
  2. If a border width has been specified by $BW, all windows will be assigned this value when focus.sh is run with them. That means that even if that value has changed while you have a fullscreen window, this window will take into account this new value when it is restored.
lwilletts commented 8 years ago

I understand your arguments to this and even agree with some of them, however, the scripts in contrib are not meant to be a 'final' solution, they are meant to inspire you to write your own scripts for your own needs. They are there to provide a 'clean' basic implementation of essential desktop functions, or showcasing an idea.

Now don't get me wrong it's great that you're thinking about these concepts: in this case, about the interaction fullscreen.sh and focus.sh have when using them with each other. One solution is source $BW from a common file for example. Have a look at fyre for an example on what I've done to solve this problem.

Also another minor thing I'd like to note is the pull request itself, it's a struggle to understand what you're trying to do on first glance when what you're changing is spaced over 6 commits and have non-descriptive comments like 'Update $scriptname' Just some food for thought and I hope you stick with wmutils :)

z3bra commented 8 years ago

Sorry for the late reply.

Your first change (in focus.sh) makes sense. It's simple, and perfectly fit the behavior you described. However, this is (I supposed) a "fix" intended for people using wmutils on top of a window manager like swm, that will set the windows border itself.

The contrib repo is intended to provide short scripts proving that wmutils can be used alone. By setting a default border, the script will work as intended out of the box, and provide windows with a default border the user can modify.

If you want to use wmutils with a WM, focus.sh will probably get in your way, but as Wildefyr said, keep in mind that it's supposed to serve as an example, not as a final solution.

Willy Goiffon

brandonpaiz commented 8 years ago

I see.

Ah well, the solution works for me. Anyway, sorry for the 6 comits, I'm new to git and wasn't thinking that they would show in the pull request. I'm a big fan of wmutils, even if it does have a few issues.

dcat commented 8 years ago

I for one agree that this needs a fix.

I have multiple windows with different border sizes, and just write my own script to suit it.

How about we implement a KEEP_BORDER environment variable that causes it to loop through the windows and use the current border width of the window?

z3bra commented 8 years ago

Yeah that might be an option. Feel free to implement it