Opened 8 years ago

Closed 3 years ago

#8188 closed enhancement (fixed)

Make Workspaces Auto-raise behave like LaunchBox Auto-raise

Reported by: devine Owned by: leavengood
Priority: normal Milestone: R1
Component: Applications/Workspaces Version: R1/Development
Keywords: workspaces, auto-raise Cc:
Blocked By: Blocking: #7101
Has a Patch: yes Platform: All

Description

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.

Attachments (4)

EnhancedRaise.diff (2.3 KB ) - added by devine 8 years ago.
Makes Workspaces auto-raise work nicer.
NewEnhancedRaise.diff (1.7 KB ) - added by devine 8 years ago.
A new version of Auto-raise enhancement for Workspaces following Axel's advice.
workspacesdiff1.patch (13.9 KB ) - added by devine 8 years ago.
Fixes the workspaces autoraise behavior and removes hardcoded decorator constants.
workspacesdiff1.2.patch (13.9 KB ) - added by devine 8 years ago.
Fixes the workspaces autoraise behavior and removes hardcoded decorator constants.

Download all attachments as: .zip

Change History (19)

comment:1 by devine, 8 years ago

Has a Patch: set

by devine, 8 years ago

Attachment: EnhancedRaise.diff added

Makes Workspaces auto-raise work nicer.

comment:2 by axeld, 8 years ago

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();
}

comment:3 by devine, 8 years ago

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

comment:4 by devine, 8 years ago

Reuploaded.

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

comment:5 by axeld, 8 years ago

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?

comment:6 by devine, 8 years ago

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

comment:7 by axeld, 8 years ago

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?

by devine, 8 years ago

Attachment: NewEnhancedRaise.diff added

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

comment:8 by devine, 8 years ago

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.

comment:9 by devine, 8 years ago

Has a Patch: unset

comment:10 by devine, 8 years ago

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

in reply to:  description comment:11 by devine, 8 years ago

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.

by devine, 8 years ago

Attachment: workspacesdiff1.patch added

Fixes the workspaces autoraise behavior and removes hardcoded decorator constants.

comment:12 by devine, 8 years ago

Has a Patch: set

by devine, 8 years ago

Attachment: workspacesdiff1.2.patch added

Fixes the workspaces autoraise behavior and removes hardcoded decorator constants.

comment:13 by leavengood, 7 years ago

Owner: changed from axeld to leavengood
Status: newin-progress

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.

comment:14 by pulkomandy, 3 years ago

Blocking: 7101 added

comment:15 by pulkomandy, 3 years ago

Resolution: fixed
Status: in-progressclosed

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

Note: See TracTickets for help on using tickets.