[haiku-bugs] Re: [Haiku] #1245: implement notification service and API

From: "stippi" <trac@xxxxxxxxxxxx>
Date: Thu, 27 May 2010 10:46:17 -0000
#1245: implement notification service and API
----------------------------------+-----------------------------------------
 Reporter:  wkornewald            |       Owner:  stippi       
     Type:  enhancement           |      Status:  in-progress  
 Priority:  normal                |   Milestone:  Unscheduled  
Component:  Kits/Application Kit  |     Version:  R1/pre-alpha1
 Keywords:                        |    Platform:  All          
Blockedby:                        |       Patch:  1            
 Blocking:                        |  
----------------------------------+-----------------------------------------
Changes (by stippi):

  * owner:  leavengood => stippi
  * status:  new => in-progress


Comment:

 Ok, here is my review. Although some remarks seem like I read everything
 very carefully, indeed I only skimmed some passages, so there may actually
 be other noteworthy things left. :-)

 All in all, the patch is great, though, and since it's so large, I think
 it will be easier if I apply it as is, and you send changes against trunk,
 which will greatly reduce the size of the patches and make further reviews
 much easier. Here we go:

 BNotification:
  * Theoretically could be entirely BMessage-based protocol.
  * Application signature could be initialized internally (calling team).
  * Uses too much pointers and manual memory management. (For examlpe:
 char* -> BString)
  * Methods to internal members should not return non-const pointers.
  * Memory management for fFile even seriously broken, takes over ownership
 of potentially temporary entry_ref passed to SetOnClickFile().
  * Casting to (void*) is never necessary.
  * Ownership of fBitmap is also unclear. fBitmap not freed in destructor,
 but ownership of other stuff is transferred, so it's confusing. It should
 make a copy.


 headers/os/Roster.h:

 {{{
 @@ -116,6 +117,10 @@ class BRoster {
                 void AddToRecentFolders(const entry_ref *folder,
                                         const char *appSig = 0) const;

 +               // notifications
 +               status_t Notify(BNotification* notification,
 +                                       int32 timeout = -1) const;
 +
                 // private/reserved stuff starts here
                 class Private;
 }}}

 Shouldn't timeout be bigtime_t? Why not "const BNotification&
 notification"?

 headers/private/notification/NotificationReceived.h is not self-contained
 (notification_type).

 status_t
 BRoster::Notify(BNotification* notification, int32 timeout) const
  * Documentation wrong: Message is delivered asynchronously.


 src/kits/notification/AppUsage.cpp
  * Code duplication in Flatten() and FlattenedSize(), should use common
 private method to build the message.
  * AppUsage::Unflatten() does not check for errors to restore fName, fRef
 and fAllow.

 src/kits/notification/NotificationReceived.cpp
  * Code duplication in Flatten() and FlattenedSize(), should use common
 private method to build the message.
  * Unflatten() needs to check return codes of BMessage::Find*() methods.

 src/preferences/notifications/GeneralView.cpp
  * Mentions "InfoPopper installation" in error alerts.
  * Would replace mentions of "the server" with "the notification service".
  * I think GeneralView::_CanFindServer() implements as fall-back mechanism
 what be_roster->FindApp() already tried.

 src/servers/notification/AppGroupView.cpp
  * AppGroupView::Draw(BRect updateRect): Doing a lot of unnecessary
 PushState() PopState() combos. Calling Sync() before returning is
 unnecessary, unless Draw() is invoked somewhere manually (where it should
 be Invalidate() instead).

 src/servers/notification/BorderView.cpp
  * Should pass B_FULL_UPDATE_ON_RESIZE to BView flags, instead of
 B_FRAME_EVENTS and overriding FrameResized().

 src/servers/notification/NotificationServer.cpp
  * NotificationServer::QuitRequested() is unnecessary, it's doing the very
 same thing that BApplication is already doing. This is only necessary if
 NotificationWindow needs the NotificationServer object to still be alive
 when it's quitting (doesn't seem to be the case).

 src/servers/notification/NotificationView.cpp
  * NotificationView::Draw() could use the BControlLook class for drawing
 the progress bar. Some PushState() PopState() combos are overhead.

  * NotificationView::MouseDown() switching on buttons is not good, since
 buttons is a bitfield. Should be if ((buttons & B_PRIMARY_MOUSE_BUTTON) !=
 0).

 src/servers/notification/NotificationWindow.cpp
  * NotificationWindow::MessageReceived() line 187: "Error" -> "error".
  * Error alerts should mention "Notification service: " before messages,
 since it will be unclear what application is generating the alert.

-- 
Ticket URL: <http://dev.haiku-os.org/ticket/1245#comment:8>
Haiku <http://dev.haiku-os.org>
Haiku - the operating system.
[haiku-bugs] Re: [Haiku] #1245: implement notification service and API

Other related posts:
