[interfacekit] Re: Phase 1

> >well, it looks like I can't stop sending longish mails. ;-)
> 
> No kidding!  Sorry about the delay in response; been kind of busy the 
> last couple of days.  Hopefully my comments won't derail things to 
> much. 
> =P

It fortunately doesn't look like that. :-)

> >Unfortunately the order is relatively strict: 3. requires 1. and 2., 
> >and 4. builds on top of 3.
> >1. and 2. can be done mostly in parallel, though at the end of 2. 
> >BMessenger will ne needed. I have no idea how well the libbe 
> > BMessenger 
> >deals with our BHandler/BLooper objects though.
> 
> I would hazard a guess that libbe's BMessenger will not deal at all 
> with 
> our BHandler/BLooper objects.  I haven't made any attempt to ensure 
> that 
> the guts of Handler/Looper registration is indentical/compatible with 
> Be's.  In short:  we'll need to implement BMessenger in order to use 
> it 
> successfully.

So it's definately a good idea, that it is the topmost working package.

> >Conventions
> >-----------
> >
> >classes BXyz and constants B_XYZ. The constants are used in the API 
> >classes implementations and therefore I suggest the common prefix 
> > REG_. 
> 
> >The classes on the other hand don't need a prefix and thus I 
> > wouldn't 
> >prepend any.
> 
> For anything that is *truly* internal to the registrar (i.e., not 
> exposed to BApplication, etc.) this would be fine with me.  Anything 
> that gets used outside of registrar should use the standard BXyz and 
> B_XYZ.  I think exposing non-standard names is a Bad Thing (tm). =)

I knew, it is impossible to have a spontaneous consensus on 
conventions. ;-)

I would like to achieve to goals:
* Avoid polluting the B_ namespace (e.g. I don't think B_SUCCESS is an 
option).
* Make the identifiers self-reflective, i.e. it should be clear, that 
they have to do with the registrar.

So, what do you think about B_REG_* or BReg::* (or BRegistrar::* for 
abbreviation phobics)?

> >For the message constant values I suggest four small letters 
> > starting 
> >with 'rg', e.g. 'rgsu' for REG_SUCCESS. The same convention goes for 
> >type constants. Note, that neither the message nor the type 
> > constants 
> >will ever get in touch with user application entities. Messages with 
> > a 
> >registrar specific "what" field or one containing a specifically 
> > typed 
> >field are only allowed for messages sent to the registrar and for 
> >synchronous reply messages to those. Such a message must never be 
> > sent 
> >to an application or any handler.
> 
> Again, all fine for registrar's internal use.

Are you content with the message/type constant values? Or do you think 
they should also lie within the range Be reserved, i.e. 'ABCD' for type 
constants and '_ABC' for message constants? As discussed, the API user 
won't get in contact with them.

> >Structures/Classes
> >------------------
> >
> >The only more or less interesting structures/classes are those in 
> > the 
> >roster, namely _TRoster_ (a friend of BMessenger), which manages the 
> >running applications. First of all, please let us rename that beast. 
> >Let's move it into the BPrivate namespace and call it just Roster 
> > (or 
> >TRoster, though I'm not sure what the T stands for -- team maybe?).
> 
> Well, at Borland (which is the only other place I've seen using the 
> 'T' 
> prefix systematically) the T stood for 'type'.  We would sometimes 
> joke 
> that it actually stood for 'turbo' (as in "Turbo C++", etc. =).

Maybe we should keep it then. ;-)

> <snipped well-thought-out RosterAppInfo exposition>
> 
> >Roster must implement an efficient team_id->RosterAppInfo and 
> > signature
> >->set of RosterAppInfo mapping. As the number of applications will 
> >quite likely be relatively small (< 100), even a single unsorted 
> > list 
> >and a linear search should be sufficient. We may want to consider 
> >binary search on sorted lists or even hash tables later.
> 
> At first I thought we'd better go with something more effecient than 
> a 
> straight list from the get-go, but I think you're right and speed 
> won't 
> be an issue here.  As long as handling of the list of apps is 
> abstracted 
> properly, we should have no problem changing the storage method later 
> if 
> necessary.

It may be a good idea to put the list management into a separate class 
that can easily be exchanged.

> >The Roster class will implement all roster features. The registrar 
> >application's MessageReceived() may dispatch the roster messages 
> >directly to the respective Roster methods. It might not even extract 
> >the contents of the message, but rather leave that for Roster.
> 
> I would likely extract the message contents into parameters for 
> Roster, 
> but that's just personal preference; I don't think there's much of a 
> technical argument to be made either way.

It came into my mind that the application's MessageReceived() will need 
to handle quite a lot of messages and extracting the fields for all of 
them at this very method would bloat it. Aside from this less code has 
to be changed, when we decide to modify a request, and it might be less 
painful to deal with optional message field. Finally, almost for all 
requests a reply needs to be sent, and directly using 
BMessage::SendReply() sounds straighter to me than to get 
BMessage::ReturnAddress() and pass it to the method.
But yes, these aren't really technical arguments.

> >Implementation
> >--------------
> >
> >status_t RegisterApp(const char *signature, const entry_ref *ref
> >                                      uint32 flags, team_id team, port_id 
> > port,
> >                                      int32 argc, const char *const *argv)
> 
> I'm guessing this replaces/merges AddApplication() and 
> xLaunchAppPrivate()?  It seems to me that it would be better to keep 
> these separate (though I don't mind renaming them).  AddApplication() 
> is 
> obviously for BApplication's use during launch, whereas the other is 
> for 
> use when remotely launching an app.  I think these are pretty 
> distinct 
> tasks (though I'm open to being convinced otherwise ;). 

Yep, they are and I meant it to be a direct replacement for 
AddApplication() only.
Some clarifications: The four functions should merely be equivalent to 
the current functions:

RegisterApp()                   == AddApplication()
RegisterRunningApp()    == CompleteRegistration()
UnregisterRunningApp()  == RemoveApp()
UnregisterApp()                 == RemovePreRegApp()

Well, actually I don't know, whether they are equivalent, because I 
haven't completely understood the original ones.
Here is how I think the registration process should work. There are two 
levels of registration, err, actually three: unregistered, registered 
and registered-running.

The BApplication constructor makes the application known to the roster 
via RegisterApp(). From that time on it appears on the roster, i.e. 
roster-watchers receive a notification and all others can use the 
BRoster functionality to query the app -- IsRunning() will return true, 
and TeamFor() and GetRunningAppInfo() will work. Registered-running is 
the level on which the application, that is its message loop, is really 
running. BApplication::Run() will call RegisterRunningApp() which in 
turn will cause the roster to send the B_ARGV_RECEIVED, B_REFS_RECEIVED 
and B_READY_TO_RUN messages.

Unregistration works simply the other way around: BApplication::Quit() 
calls UnregisterRunningApp() and the level is just registered from then 
on (another call to Run() is possible thereafter!). Finally the 
BApplication destructor calls UnregisterApp().

> In particular 
> these

> >params:
> >- argc: The application's command line parameter count.
> >- argv: The application's command line parameters.
> 
> don't make any sense when BApplication is registering itself -- why 
> would BRoster care what these are, since the app already knows?

I think, it makes sense. Or at least it makes things easier. If the 
roster sends B_ARGV_RECEIVED, then those arguments must get there 
somehow. Well, you might say, that the application could as well send 
the message itself or just call ArgvReceived(). OK, then this argument 
may be more convincing: If the application is single/exclusive launch 
and called the second time, then the argvs must be sent to the already 
running instance. If the latter is on level registered, not registered-
running, then it might be better to wait with sending the argvs until 
its message loop is running. The roster can do that, the new 
application instance can't.

To summarize my point: The roster can much better deal with the argv 
stuff, and let it do so makes the BApplication side a lot simpler.

> >return:
> >- REG_ALREADY_REGISTERED: For single/exclusive launch applications
> >  that are launched the second time.
> 
> As stated before, I think this should use the B_* convention.

Point already taken. :-)

> >status_t RegisterRunningApp(team_id team, thread_id thread)
> >
> >Tells the roster, that the application is now going to run the 
> > message 
> >loop. The roster sends the command line arguments as a 
> > B_ARGV_RECEIVED 
> >message and if applicable also a B_REFS_RECEIVED and other initial 
> >messages and finally a B_READY_TO_RUN.
> >
> >params:
> >- team: The application team.
> >- thread: The application looper thread.

A correction: The thread argument is not needed here, but in 
RegisterApp(). It is not the looper thread, but the application's main 
thread (which may differ). The same of course goes for the message 
protocols.

> >
> >return:
> >- B_OK: :-))
> >- ...
> 
> It seems to me that this function is intended to effect the result of 
> the BRoster::IsRunning() functions, right?

No, see above.

> I just ran some tests with 
> some interesting results:
> 
> thread_id TestApp::Run()
>         IsRunning():       true
>         fReadyToRunCalled: false
> void TestApp::ReadyToRun()
>         IsRunning():       true
>         fReadyToRunCalled: false
> TestApp::MessageReceived() -- 1234
>         IsRunning():       true
>         fReadyToRunCalled: true
> 
> I copied Application.h locally and included it, adding my 
> BApplication-derived class as a friend (so I could get at 
> fReadyToRunCalled).  Although I overloaded MessageReceived() 
> explictly 
> to catch the B_READY_TO_RUN message, it never came through.
> I guess 
> Be's implementation does its work directly without ever sending the 
> message.  I send a message with '1234' as the what from ReadyToRun() 
> -- 
> fReadyToRunCalled has be set by then.

AFAIK most system messages don't show up in MessageReceived(). The 
hooks are either called by DispatchMessage() or even earlier.

> The whole point of the exercise is to illustrate that 
> BRoster::IsRunning() returns true before B_READY_TO_RUN is 
> (theoretically) sent and certainly before ReadyToRun() is called by 
> BApplication.  In short, I think RegisterRunningApp() needs a 
> rethink. 
> ;)
> 
> Please correct me if I'm misunderstanding. =)

I hope my attempt for clarification helps.

> >--------------------------------------------------------------------
> > ---
> >
> >status_t UnregisterRunningApp(team_id team)
> 
> I assume this function performs the opposite role of 
> RegisterRunningApp(), namely setting the result of 
> BRoster::IsRunning() 
> to false once the message loop is done.

Opposite role of RegisterRunningApp() yes, BRoster::IsRunning() no. :-)

> An extension of the test 
> mentioned above reveals this:
> 
> bool TestApp::QuitRequested()
>         IsRunning():       true
>         fReadyToRunCalled: true
> TestApp::~TestApp()
>         IsRunning():       true
>         fReadyToRunCalled: true
> 
> The overall conclusion I draw from all this is that 
> BRoster::IsRunning() 
> will return true from the time BApplication initially registers 
> itself 
> until it is destroyed in ~BApplication(), hence there is no need to 
> Register/Unregister the running state of the application.  This, of 
> course, has implications for the RosterAppInfo struct.

I'm not sure, if it is really necessary to have a two-level 
registration, but it seems to me at least convenient. For sure it gives 
the roster more knowledge, that can be exploited -- e.g. avoid sending 
an application not running the message loop messages.

> >--------------------------------------------------------------------
> > ---
> >
> >status_t UnregisterApp(team_id team)
> >
> >Unregisters the application from the roster. Called from the 
> >BApplication destructor.
> 
> Amazingly, I have no bones to pick here. ;)

Lucky me. :-)

> >Protocols
> >
> >reply:               - REG_SUCCESS
> >reply:               - REG_ERROR
> >message:     REG_GET_MIME_MESSENGER/REG_GET_CLIPBOARD_MESSENGER
> >  - REG_ALREADY_REGISTERED: For single/exclusive launch applications
> >    that are launched the second time.
> >message:     REG_UNREGISTER_APP
> >message:     REG_GET_APP_LIST
> >message:     REG_GET_RUNNING_APP_INFO
> >message:     REG_ACTIVATE_APP
> 
> Again, as long as these messages are truly internal to registrar and 
> BRoster, these names are fine with me.

Yep.

> >message:     REG_REGISTER_RUNNING_APP
> >message:     REG_UNREGISTER_RUNNING_APP
> 
> Unless I was horrendously mistaken earlier, these two messages should 
> not be needed.

If we drop the corresponding methods, then I agree. But currently I 
still like the idea of a two-level registration.

CU, Ingo

Other related posts: