Make Workspaces Auto-raise behave like LaunchBox Auto-raise

[devine]

Previously you could not just let the Workspaces applet snap to the edge of the screen and expect auto-raise to work. LaunchBox did not have this problem and that's why I first thought that Auto-raise was completely broken! I've come up with a fix.

[devine]

Makes Workspaces auto-raise work nicer.

[axeld]

Thanks! You introduce a number of coding style violations with your patch, though, and it much more complicated than needed. Besides that, you should use the BWindow::GetDecoratorSettings() to determine the actual window bounds -- what you do there looks a bit messy.

The code should a bit look like this (pseudo code):

BScreen screen(this);
BRect screenFrame = screen.Frame();
if (mouse on screen edge) {
	BRect tabFrame;
	BMessage settings;
	if (GetDecoratorSettings(&settings) == B_OK)
		tabFrame = settings.FindRect("tab frame");

	screenFrame.InsetBy(tabFrame);
	if (screenFrame.contains(Frame())
		Activate();
}

[devine]

Just noticed the patch is incorrect as it will not work when the window decoration is turned off. Will re-upload soon.

[devine]

Reuploaded.

I'm *really* sure it is right this time.

[axeld]

Thanks for the quick rework! There are still a lot of coding style violations, just a few hints:

• Don't use useless parenthesis "-(offset)" can just be written as "-offset"
• There is a blank before '{'.
• The first "if" is indented incorrectly.
• Using the form:

  if (!x)
  	return;
  else {
  	[...]
  }
  

  is not good style: the "else"-block is completely superfluous and misleading. It should be written like this instead:

  if (!x)
  	return;
  
  [...]
  

• Write comments either before a block, or with an extra indent on the next line like this:

  x = 1 + 2;
  	// This is so complicated because I only have two fingers
  

  or:

  // Take the tab height into account
  if (...) {
  	...
  }
  

Also, I have a few comments on the contents still:

• Instead of kScreenBorderOffset (where does that come from, why /2?) please use the border size as returned by the decorator settings message (I guess it's returned by FindInt32("border size"), but it might be different).
• The check windowFrame.Contains(where) should be moved to the top condition, as proceeding with the rest is an worthless effort then. However, it's wrong, too, as the window frame doesn't contain the border, so it should always fail.
• The 'else' case if GetDecoratorSettings() failed is the same as the block directly above; always strive to avoid code duplication.

Have you tested if your code actually works?

[devine]

It certainly does work, perfectly.

I agree about the if/else statements - I'd better stick with my usual style ;)

I should have guessed there should have been a way to get the border size. However I'm sort-of flying blind without the documentation. I'll poke into the code if your suggestion isn't quite right. kScreenBorderOffset is used elsewhere in the application to describe the width of window frame * 2.

I can move the windowFrame.Contains(where) to the top as that does make sense, however the logic I have there is correct. I am extending the Window frame out to meet the screen frame -- the mouse also has to be at the edge of the screen for the auto-raise to be triggered. Therefore they all contained in the same place (note that two BRects of the *same size* contain eachother).

[axeld]

Indeed, I missed that you change windowFrame, sorry!

I don't know the contents of the decorator settings message off-hand either, but a settings.PrintToStream() should help you further. While you're at it, you could eliminate the kScreenBorderOffset variable altogether, as it's decorator dependent, and not really a constant.

Also, one thing I don't understand in your patch are the comments about SAT - it shouldn't really change anything that matters here, or am I missing something?

[devine]

A new version of Auto-raise enhancement for Workspaces following Axel's advice.

[devine]

You can do a PrintToStream() on settings? Useful!

When you turn off the decor/tab GetDecoratorSettings() != B_OK and you only need one offset size.

I will go through and replace all the kScreenBorderOffset stuff I might be able to remove the need for the disabled decorator clause.

[devine]

#8194 should get sorted out before this one. It gets rid of the decorator constants. I have also come up with a new, cleaner way of fixing this auto-raise problem too, but I'll wait.

[devine]

I have a patch which solves #8188 and #8194. The reason it took so long was because I wanted to see if I could do a git pull request, and I also found a nasty little bug in my patch.

[devine]

Fixes the workspaces autoraise behavior and removes hardcoded decorator constants.

[devine]

Fixes the workspaces autoraise behavior and removes hardcoded decorator constants.

[leavengood]

Hi Daniel,

Thanks a lot for your patch.

I've applied it locally and have fixed quite a few whitespace problems and a few other minor things, but beyond that I notice that in the case of there not being any workspace settings, the resulting window is quite tiny and positioned at the top left of the screen (with the tab off the screen), as opposed to the bottom right. Also the code used for Zoom() is quite similar to the resizing code in the constructor, except something about the latter isn't right. I'm willing to dig into it more and fix the issues, I just figured I'd leave a message here in case you might know better than I what is happening.

Also I'll take ownership of this ticket.

[pulkomandy]

Fixed in hrev50549. I couldn't reproduce the issues ryan mentions.