Opened 7 years ago

Closed 2 years 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 7 years ago.
0001-Notification_Server-Added-ability-to-choose-position.patch (14.2 KB ) - added by hrily 2 years ago.
Patch with fixes for 64bit build

Download all attachments as: .zip

Change History (33)

by Giova84, 7 years ago

comment:1 by dsjonny, 7 years ago

+1

comment:2 by pulkomandy, 5 years ago

Milestone: R1Unscheduled

comment:3 by pulkomandy, 3 years ago

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 by hrily, 2 years ago

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 by pulkomandy, 2 years ago

Yes, this sounds right.

Pointers to useful parts of the sourcecode:

comment:6 by hrily, 2 years ago

Thank you!!! :)

That will save my extra work.

comment:7 by hrily, 2 years ago

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 by pulkomandy, 2 years ago

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

comment:9 by hrily, 2 years ago

Okay

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

comment:10 by hrily, 2 years ago

Also

How can I test the notifications???

comment:11 by Janus, 2 years ago

Command notify

comment:12 by hrily, 2 years ago

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 by Janus, 2 years ago

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 by pulkomandy, 2 years ago

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 by hrily, 2 years ago

Okay

Thank you!

Seems like interesting problem to solve... :D

comment:16 by hrily, 2 years ago

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 by pulkomandy, 2 years ago

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 by hrily, 2 years ago

Has a Patch: set

comment:19 by hrily, 2 years ago

@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 by pulkomandy, 2 years ago

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 by hrily, 2 years ago

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 by pulkomandy, 2 years ago

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 by hrily, 2 years ago

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 by pulkomandy, 2 years ago

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

comment:25 by hrily, 2 years ago

Okay

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

Last edited 2 years ago by hrily (previous) (diff)

comment:26 by pulkomandy, 2 years ago

Just B_FOLLOW_DESKBAR, the others already have the right names?

comment:27 by hrily, 2 years ago

I've added the patch.

Please review...

comment:29 by ohnx56, 2 years ago

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

by hrily, 2 years ago

Patch with fixes for 64bit build

comment:30 by hrily, 2 years ago

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 by pulkomandy, 2 years ago

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.