Opened 5 years ago

Closed 6 months ago

#9749 closed enhancement (fixed)

Notification_Server: add the ability to choose the position of notifications (easy)

Reported by: Giova84 Owned by: pulkomandy
Priority: normal Milestone: Unscheduled
Component: Preferences/Notifications Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: x86

Description

Currently, notifications are displayed where the Deskbar is placed, but would be nice have the ability to choose (in Notifications preflet) where to show notifications, maybe in the four corners of the screen. Mockup attached.

Attachments (2)

Notification_Pref_Mockup.png (14.8 KB) - added by Giova84 5 years ago.
0001-Notification_Server-Added-ability-to-choose-position.patch (14.2 KB) - added by hrily 7 months ago.
Patch with fixes for 64bit build

Download all attachments as: .zip

Change History (33)

Changed 5 years ago by Giova84

comment:1 Changed 5 years ago by dsjonny

+1

comment:2 Changed 4 years ago by pulkomandy

Milestone: R1Unscheduled

comment:3 Changed 16 months ago by pulkomandy

Keywords: Notification_Server add the ability to choose the position of notifications removed
Summary: Notification_Server: add the ability to choose the position of notificationsNotification_Server: add the ability to choose the position of notifications (easy)

comment:4 Changed 7 months ago by hrily

I would like to contribute to this issue.

If I'm not wrong, I'll need to figure out:

+ How to change the position of the notification

+ Where are the preference values saved, and how to add one

+ How to add new field to the Preferences GUI

comment:5 Changed 7 months ago by pulkomandy

Yes, this sounds right.

Pointers to useful parts of the sourcecode:

comment:6 Changed 7 months ago by hrily

Thank you!!! :)

That will save my extra work.

comment:7 Changed 7 months ago by hrily

Finally Booted Haiku.

I had few doubts:

+ Should I add the functionality to change the icon size (as seen in the mockup), since it's not currently present?

comment:8 Changed 7 months ago by pulkomandy

No, this was removed from the settings since the mockup was created. The small 16x16 icon was not visible enough.

comment:9 Changed 7 months ago by hrily

Okay

So should I create extra tab viz "Display", or should I keep the "Position" inside "General" tab?

comment:10 Changed 7 months ago by hrily

Also

How can I test the notifications???

comment:11 Changed 7 months ago by Janus

Command notify

comment:12 Changed 7 months ago by hrily

One more doubt

If the user chooses the same corner as deskbar, should we show the notification over the deskbar or besides it??

comment:13 Changed 7 months ago by Janus

In my opinion it should work as it works now if is in the corner of the deskbar...

It would be nice to have the option "follow the deskbar" and keep the actual code, but I don't know what the other think

comment:14 Changed 7 months ago by pulkomandy

Yes, the current behavior of "near the deskbar" should be kept and be the default.

And if the user choses the same corner as deskbar, the notification should still be besides deskbar; not over or under it.

You should keep the position in the general tab.

comment:15 Changed 7 months ago by hrily

Okay

Thank you!

Seems like interesting problem to solve... :D

comment:16 Changed 7 months ago by hrily

Hie

I've implemented following behavior:

+ If user specifies same corner as deskbar, then show notification besides it.

+ If user specifies same position when deskbar is at top or bottom, then show notification below or above the deskbar.

+ If user specifies "Follow Deskbar", then follow original behavior.

+ If user specifies corner other than deskbar, then show notification on that corner.

Please confirm this, I'll send the patch if this is okay...

comment:17 Changed 7 months ago by pulkomandy

That sounds right, but we want to review the code and see if you did not miss any edge case (such as what happens if the setting is currently at the same corner as deskbar, but then deskbar is moved elsewhere).

So, please send the patch :)

comment:18 Changed 7 months ago by hrily

Has a Patch: set

comment:19 Changed 7 months ago by hrily

@PulkoMandy

There wasn't any comment after the patch upload. So I thought I should drop a message.

Did you start reviewing the code?

comment:20 Changed 7 months ago by pulkomandy

Sorry, I forgot about it. The logic looks fine, but there are some style problems.

"loc" is not a good name for a variable. Use "location". I would consider reusing the existing B_FOLLOW_* constants instead of introducing new ones?

comment:21 Changed 7 months ago by hrily

Style - I used checkstyle.py, the result was fine. Can you specify?

Location - I will.

B_FOLLOW_* - I didn't get this part...

comment:22 Changed 7 months ago by pulkomandy

The only style issue (that I can see) is the name of the "loc" variable.

For the B_FOLLOW_* things: we already have B_FOLLOW_LEFT, B_FOLLOW_TOP, etc constants. I think it would make sense to use these instead of creating a new enum with B_NOTIFICATION_LOWER_RIGHT, etc.

So you would use B_FOLLOW_LEFT | B_FOLLOW_BOTTOM for a "lower left" setting, for example.

comment:23 Changed 7 months ago by hrily

Really sorry for the delayed reply. My laptop had a little issue the day of last reply.

I'll send a new patch with these changes asap...

One doubt: What should we use for "Follow Deskbar" option. (I think B_FOLLOW_NONE should do...)

comment:24 Changed 7 months ago by pulkomandy

Yes, maybe have a #define B_FOLLOW_DESKBAR B_FOLLOW_NONE so we can understand it more easily when reading the code?

comment:25 Changed 7 months ago by hrily

Okay

Should I do this for all the settings, or just B_FOLLOW_DESKBAR?

Last edited 7 months ago by hrily (previous) (diff)

comment:26 Changed 7 months ago by pulkomandy

Just B_FOLLOW_DESKBAR, the others already have the right names?

comment:27 Changed 7 months ago by hrily

I've added the patch.

Please review...

comment:29 Changed 7 months ago by ohnx56

Looks like it was fixed by waddlesplash already. (hrev51706)

Changed 7 months ago by hrily

Patch with fixes for 64bit build

comment:30 Changed 7 months ago by hrily

Sorry for the trouble.

Was travelling few days so couldn't reply.

I see that it is fixed already, good to see that...

I had a great time solving this.

comment:31 Changed 6 months ago by pulkomandy

Resolution: fixed
Status: newclosed

Closing again as the 64bit fix has already been done.

Note: See TracTickets for help on using tickets.