Opened 10 years ago

Closed 10 years ago

#4721 closed bug (fixed)

app_server flag fixes (patch)

Reported by: rogueeve Owned by: stippi
Priority: normal Milestone: R1
Component: Servers/app_server Version: R1/alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

Patch candidate for some app_server issues having to do with B_NO_SERVER_SIDE_WINDOW_MODIFIERS, as discussed on the haiku-dev mailing list. The second file is a program that can be used to test the changes. Changeset is:

  • Fixed B_NO_SERVER_SIDE_WINDOW_MODIFIERS and B_CLOSE_ON_ESCAPE window flags not working.
  • Added B_VALID_WINDOW_FLAGS constant to Window.h and modified app_server's ValidWindowFlags() function, to eliminate the need for a list of flags to be in two places, and possibly avoid the above problem in the future.
  • The same method unfortunately would not work for the WINDOW_FEEL and _LOOK constants, because they are not bitflags, so a warning was added to Window.h about the possible need to update app_server if they are changed (a bit of a todo, I guess)
  • CTRL+ALT-click "server side window modifiers" now require exactly the CTRL+ALT shortcut to be down; CTRL+ALT+SHIFT for example is not counted.
  • CTRL+ALT clicks are ignored and let through to the window if no special action would have been taken anyway; e.g. if the window is not movable.

Attachments (3)

serversidetest.zip (53.1 KB ) - added by rogueeve 10 years ago.
program which tests the above functionality
app_server_flags_4721.patch (3.7 KB ) - added by rogueeve 10 years ago.
app_server_flags_4721_nowindowmod.patch (3.5 KB ) - added by rogueeve 10 years ago.
same patch as above, but does not add B_VALID_WINDOW_FLAGS

Download all attachments as: .zip

Change History (12)

by rogueeve, 10 years ago

Attachment: serversidetest.zip added

program which tests the above functionality

comment:1 by leavengood, 10 years ago

Owner: changed from axeld to leavengood

Hi Caitlin,

Thanks for the patch, there are several style violations and I'm going to be more strict now that you've gotten a few patches committed. I know it is a long read, but please be sure your code follows our Coding Guidelines. While we don't all agree on each and every guideline, we all do follow them :)

In this patch some problems are:

  • lines 783-785, I think there are extra parenthesis and you moved the && operator to the end of the line. The operator should be at the beginning of the next line.
  • line 806, same problem with the && operator.
  • line 919 is now longer than 80 characters (just barely at 81, but we are strict.) Please put it back as it was.

When you make these changes please update the patch and leave a comment so I get notified.

comment:2 by rogueeve, 10 years ago

Updated patch to correct styling violations.

by rogueeve, 10 years ago

Attachment: app_server_flags_4721.patch added

comment:3 by jackburton, 10 years ago

Thanks for your work! I only have one remark: since there is no need for window valid flags to be public, I'd avoid the B_VALID_WINDOW_FLAGS in Window.h, and use instead a kValidWindowFlags, keeping it private.

comment:4 by rogueeve, 10 years ago

Yes you're right, and that was something I was concerned about as well. I found "headers/build/private/interface/WindowPrivate.h", which I assume would be the proper place for "kValidWindowFlags". However, that still necessitates the matching list of flags across two source files, possibly leading to someone adding another flag later and forgetting to also add it into WindowPrivate.h. If modifying the public presentation of the global <Window.h> isn't acceptable I could add an #ifdef around B_VALID_WINDOW_FLAGS such that it is only defined when included by the app server, but that seems pretty ugly. Or I could just add a warning comment about the need to update 2nd list, as was done with the FEEL and LOOK constants.

comment:5 by stippi, 10 years ago

"Or I could just add a warning comment about the need to update 2nd list, as was done with the FEEL and LOOK constants."

Yes, that would be my approach as well.

by rogueeve, 10 years ago

same patch as above, but does not add B_VALID_WINDOW_FLAGS

comment:6 by rogueeve, 10 years ago

Added another version of the patch which omits B_VALID_WINDOW_FLAGS in favor of just a warning.

comment:7 by stippi, 10 years ago

Owner: changed from leavengood to stippi
Status: newassigned

comment:8 by stippi, 10 years ago

I wonder why we even need IsValidWindowFlags() on the server side... The warnings in the header don't really belong there, but I have no better idea. Anyway, I applied your patch here and am going to test a bit. What I don't like so much is that the system modifiers can be used if the window is not movable, I didn't apply that part. That's just some weird mix-up. An application developer should consciously control this via the provided window flag.

comment:9 by stippi, 10 years ago

Resolution: fixed
Status: in-progressclosed

Apparently, the important parts of this patch were commited, closing. Thanks again!

Note: See TracTickets for help on using tickets.