[haiku-commits] Re: r36952 - in haiku/trunk: headers/os/app src/bin src/kits/app src/servers/notification
- From: Stephan Assmus <superstippi@xxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Thu, 27 May 2010 22:45:34 +0200
On 2010-05-27 at 21:50:59 [+0200], Karsten Heimrich <host.haiku@xxxxxx> wrote:
> On 27/05/2010 19:50, superstippi@xxxxxx wrote:
> > Author: stippi
> > Date: 2010-05-27 19:50:12 +0200 (Thu, 27 May 2010)
> > New Revision: 36952
> > Changeset: http://dev.haiku-os.org/changeset/36952/haiku
> >
> > Modified:
> > haiku/trunk/headers/os/app/Notification.h
> > haiku/trunk/headers/os/app/Roster.h
> > haiku/trunk/src/bin/notify.cpp
> > haiku/trunk/src/kits/app/Notification.cpp
> > haiku/trunk/src/kits/app/Roster.cpp
> > haiku/trunk/src/servers/notification/NotificationView.cpp
> > Log:
> > * Improved BNotification API.
> > - No more manual memory management.
> > - Make it clear who keeps or releases ownership of arguments passed.
> > - Copy icon, arguments and entry_refs.
> > - Do not expose implementation details (What do the BLists contain?!).
> > - BRoster takes const BNotification& and bigtime_t timeout.
> >
> > * BRoster::Notify():
> > - Proper error handling.
> > - Fixed documentation.
> >
> > * Adjusted notify:
> > - Renamed fOk to fHasGoodArguments.
> > - The "const char*" members were really "char*" members
> > (self-managed).
> > - free() is NULL-safe.
> > - fRefs contains BEntries, so passing void* to delete does no good.
> > - Adjustments to the changed API.
> > - Coding style fixes.
> >
> > * notification_server:
> > - Adjustment to the new type for timeout.
> >
> >
> > Modified: haiku/trunk/headers/os/app/Notification.h
> > ===================================================================
> > --- haiku/trunk/headers/os/app/Notification.h 2010-05-27 17:08:47 UTC
> > (rev 36951)
> > +++ haiku/trunk/headers/os/app/Notification.h 2010-05-27 17:50:12 UTC
> > (rev 36952)
> > @@ -7,12 +7,10 @@
> >
>
> < cut >
>
> >
> > +
> > class BNotification {
> > public:
> > BNotification(notification_type type);
> > @@ -47,29 +47,33 @@
> > const char* OnClickApp() const;
> > void SetOnClickApp(const char* app);
>
> (1) I wonder if we shouldn't use const BString& string while introducing
> new API's? I know all the old API's use const char*, but memory an speed
> are not really a constraint here, no?
I am usually in favor of that since you introduced the COW BString backend.
However in this case, these methods can actually unset the members by passing
NULL. Granted, it's not different from passing "" or an empty BString. I can
definitely change it, it wouldn't make a difference besides avoiding string
copies.
> >
> > - entry_ref* OnClickFile() const;
> > - void SetOnClickFile(const entry_ref* file);
> > + const entry_ref* OnClickFile() const;
> > + status_t SetOnClickFile(const entry_ref* file);
> >
> > - BList* OnClickRefs() const;
> > - void AddOnClickRef(const entry_ref* ref);
> > + status_t AddOnClickRef(const entry_ref* ref);
> > + status_t AddOnClickRef(const entry_ref& ref);
>
> I wonder what's the point here in having two different signatures? I
> would expect the pointer one to take ownership of the passed entry_ref,
> while the reference one does a copy of the argument. Same for all other
> entry_ref* signatures.
I don't agree that passing any const parameter signifies that the ownership
is transfered. However having two versions is probably still bogus. I was
wondering about it and then thought "ah... what the heck". :-)
>
> > + int32 CountOnClickRefs() const;
> > + const entry_ref* OnClickRefAt(int32 index) const;
> >
> > - BList* OnClickArgv() const;
> > - void AddOnClickArg(const char* arg);
> > + status_t AddOnClickArg(const char* arg);
>
> Same as (1).
True.
> > BNotification::BNotification(notification_type type)
> > :
> > fType(type),
> > - fAppName(NULL),
> > - fTitle(NULL),
> > - fContent(NULL),
> > - fID(NULL),
> > - fApp(NULL),
> > fFile(NULL),
> > fBitmap(NULL)
> > {
> > - fRefs = new BList();
> > - fArgv = new BList();
> > }
> >
>
> I wonder why you removed the initialisation of the class members?
> Suppose something like this:
>
> Foo::SomeFunction()
> {
> BNotification notice(B_INFORMATION_NOTIFICATION);
> BRoster().Notify(notice);
> }
>
> Wouldn't that lead to copy garbage in the Rosters Notify() function for
> e.g. Application(), Title() etc. ?
Jérôme already replied to that one... :-)
Thanks for reviewing, I guess I will make the changes you proposed.
Best regards,
-Stephan
Other related posts:
