[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

>Tasks
>-----
>
>This is basically a reincarnation of the feature list I sent earlier. 
>Closely related items are bundled into work packages. Though it might 
>work out well, if two guys work on a package at the same time, it is 
>probably simpler, if a package is dealt with by one person only.

<list snipped>

This all looks good to me.

>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.

>Tests
>-----
>
>Usually I prefer to write the tests before the implementation, but I 
>would relax the rule a bit. Writing the BMessenger tests first for 
>instance would hold all other packages. Thus I think, it is reasonable 
>to write the tests for a package after implementing it.

Either way is fine for me.  I don't think the delay will be serious 
enough to cause problems.

>Originally I intended to list some test ideas for each packages, but 
>it's actually straight forward: All the features implemented by a 
>package need to be tested. ;-)

Bingo.

>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). =)

>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.

>Error constants start at B_ERRORS_END + 1.

Good practice.

>To not interfere with the BeOS registrar I propose the names 
>"_obos_roster_thread_" and "_obos_roster_port_" for the registrar's 
>main thread and app looper port respectively. The names should be 
>defined as static variables in a common header file (Registrar.h). The 
>other shared definitions should be located there as well.

Great idea.

>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. =).

<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.

>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.

>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 ;).  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?

>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.

>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.
>
>return:
>- B_OK: :-))
>- ...

It seems to me that this function is intended to effect the result of 
the BRoster::IsRunning() functions, right?  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.

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. =)

>-----------------------------------------------------------------------
>
>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.  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.

>-----------------------------------------------------------------------
>
>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. ;)

>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.

>message:       REG_REGISTER_RUNNING_APP
>message:       REG_UNREGISTER_RUNNING_APP

Unless I was horrendously mistaken earlier, these two messages should 
not be needed.

I've attached my files for the test app I talked about earlier, just to 
make sure that other eyes can make sure I wasn't too high on crack when 
I wrote it. =P

e

Necessity is the plea for every infringement of human freedom. It is the 
argument of tyrants; it is the creed of slaves.
        -William Pitt, British prime-minister (1759-1806)

-- Binary/unsupported file stripped by Ecartis --
-- Type: text/x-source-code
-- File: LocalApplication.h


-- Binary/unsupported file stripped by Ecartis --
-- Type: application/x-be_attribute
-- File: For BeOS Use Only



-- Binary/unsupported file stripped by Ecartis --
-- Type: text/x-source-code
-- File: main.cpp


-- Binary/unsupported file stripped by Ecartis --
-- Type: application/x-be_attribute
-- File: For BeOS Use Only

Other related posts: