Wow! Many thanks for this great review, Mark. It's incredibly helpful. I'll certainly implement most of what you mention, and will give serious thought to one or two items (e.g. putting all .application statements in the Startup file) but before deciding will get back to you to bounce some ideas off you.
 
Thx again, and best regards,
Oliver
 


From: Mark Miesfeld [mailto:miesfeld@gmail.com]
Sent: 03 October 2011 18:18
To: Open Object Rexx Users
Subject: [Oorexx-users] Small review of ooDialog User Guide examples

Hi Oliver,
 
I spent some (little) time reviewing the user guide examples so far.  Here are some general comments.
 
1.)  In exercise 4 when the user clicks on the 'Print ...' menu item, the print method is not implemented so the user gets a message dialog saying: PRINT is not a method of a CUSTOMERVIEW.
 
In a real application, this message would be confusing to a normal user.  Especially if the user was not a Rexx programmer familiar with ooDialog.
 
Rather than rely on the default behavior of ooDialog to put up a message box during the Unknown() method processing, I think it is a better practice to include stub functions for methods not implemented.  In this case something like:
 
  /*-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
    Print - Not implemented yet                                             --*/
  ::METHOD print unguarded
    msg = "The 'Print...' menu item is not yet implemented."
    ret = MessageDialog(msg, self~hwnd, '*CustomerName*', 'WARNING')
 
There are a number of advantages to this, not the least of which is that it indicates the programmer knows what he is doing.  You can use a message that is meaningful.  You can use an icon that shows the behavior is not an error, it is something the programmer has intended.  You can match the title of the message box to the title of the application rather than being 'Error.'  Etc..
 
You do explain the behavior in the user guide itself, so it is fine with me to leave this exercises the way it is if you prefer.  However, it is highly likely that people will execute the dialog as is without ever reading the user guide.
 
2.) In exercise 05, if the user presses the escape key the dialog does not close.  Worse, if the user presses the 'Close' icon in the title bar the dialog does not close.  Even worser <grin>, if the user uses the system menu and selects the 'Close Alt-F4' menu item, the dialog does not close.
 
A large portion of what makes Windows easy to use is that behaviour of applications is the same.  Microsoft provides guidelines to help developers adhere to best user experience practices.  On dialogs they say:
 
Pressing the Esc key always closes an active dialog box.  The bolding is from Microsoft.  They have other guidelines concerning the 'Close' icon, namely that it always closes the window.
 
Here again, I see you explain this behaviour, to some degree, in the user guide text.  So, if you want to keep this exercise as is, that's fine. 
 
I think it would be better though to put up a message box explaining why the dialog did not close, as would be expected.  I think the topic of how to intercept the canceling of a dialog is a good topic to cover.  Perhaps you could expand that section somewhat.  Maybe explain that when designing dialogs it is also important to preserve expected behavior on Windows.  But, here is how to change the default behavior. ... Reasons for stopping the user from closing the dialog with cancel would be ... If there is unsaved data that the user will lose.  ...
 
Again, people, like me, will run these samples without reading the doc.  They will expect the dialog to close when they close it.
 
3.) Your use of .Application is incorrect and / or misleading.  In exercise 6 you have .Application~useGlobalConstDir() and .Application~setDefaults(...) in multiple files.  But, exercise 6 is *one* application.  The .application object is used to set defaults for the *entire* application.
 
68 5:    .Application~useGlobalConstDir("O","Customer\CustomerListView.h")
88 5:    .Application~setDefaults("O", "Customer\CustomerView.h", .false)
70 5:    .Application~useGlobalConstDir("O","Order\OrderFormView.h")
... and more
 
You should have one statement in Startup.rex:
 
.Application~setDefaults("O", "OrderMgmt\OrderMgmtBaseView.h", .false)
 
and really, I think a better approach for ooDialog, for a single application, is to just use one .dll and one symbol definition file (.h file.)  I would put all dialog definitions in one .rc, however you could still keep your modular approach by having multiple .rc files for defining different dialogs and then compile all the .rc files into 1 .dll file.  All the symbol defines would go into 1 .h file that is 'included' in each individual .rc file.  Or, keep all the individual .h files but combine them into 1 .h file for use by ooDialog.
 
Nevertheless, with your current structure, you should have the single .Application~setDefaults() in Startup.rex and add all your different .h files to the global constDir.  I would add them in Startup.rex, although you could add them in the individual program files if you wanted.
 
Your Startup.rex could look like this:
 
/***********************************************
  This file is the "application" or "root" or
  "starter" part of the sample Order Management
  application.
 ***********************************************/
.Application~setDefaults("O", "OrderMgmt\OrderMgmtBaseView.h", .false)
.Application~addToConstDir("Customer\CustomerListView.h")
.Application~addToConstDir("Customer\CustomerView.h")
.Application~addToConstDir("Order\OrderFormView.h")
.Application~addToConstDir("Order\OrderListView.h")
.Application~addToConstDir("Order\OrderView.h")
.Application~addToConstDir("Product\ProductListView.h")
.Application~addToConstDir("Product\ProductView.h")
 
parse arg pwOption
...
 
although I myself would do this:
 
/***********************************************
  This file is the "application" or "root" or
  "starter" part of the sample Order Management
  application.
 ***********************************************/
.Application~setDefaults("O", "orderManagement.h", .false)
 
parse arg pwOption
...
 
with all my symbol defines in orderManagement.h
 
You could leave your .Application invocation in all your individual files if you want.  Add this to Startup.rex:
 
/***********************************************
  This file is the "application" or "root" or
  "starter" part of the sample Order Management
  application.
 ***********************************************/
.Application~setDefaults("O", , .false)
 
parse arg pwOption
...
 
and change all your individual .Application~ invocations in the other files to this:
 
.Application~addToConstDir(<fileName>)
 
 
4.) In exercise 6, I wouldn't use interpret.  Why not just put the class object in the record and invoke the the newInstance method directly on the class?
 
In ooRexx, there are rarely any times that interpret needs to be used.
 
a.) You have this in showModel():
 
    className = record~ID
    say "OrderMgmtView-showModel-01: className =" className
Which shows on the screen as:
 
OrderMgmtView-showModel-01: className = OrderList
 
but actually there is no class named OrderList.  So, that's a little misleading. <grin>
 
b.) You have this code in method showModel()
 
    use arg record    -- record is a directory object.
    className = record~ID
    say "OrderMgmtView-showModel-01: className =" className
    viewClassName = className||"View"
    interpret "."||viewClassName||"~newInstance(self)"
    say "OrderMgmtView-showModel-02:"
Instead of making the ID attribute of the directory object a string value, just use the class object.  Then the code can be simplified to:
 
    use arg record    -- record is a directory object.
    say "OrderMgmtView-showModel-01: className =" record~ID~id
    record~ID~newInstance(self)
    say "OrderMgmtView-showModel-02:"

just change you record init method to this:
 
  ::METHOD initRecords PRIVATE
    expose records
    records = .array~new()
 
    rec = .directory~new
    rec~ID = .CustomerListView
    rec~name = "Customer List"
    records[1] = rec
 
    rec = .directory~new
    rec~ID = .ProductListView
    rec~name = "Product List"
    records[2] = rec
        ...
 
 
c.)  To make the output a little prettier, put your class names in quotes.  Then instead of this:
 
OrderMgmtView-02-onDoubleClick: Record ID = The ORDERLISTVIEW class
OrderMgmtView-showModel-01: className = ORDERLISTVIEW
.OrderListView-newInstance-01: root = an ORDERMGMTVIEW
 
you will see this:
 
OrderMgmtView-02-onDoubleClick: Record ID = The OrderListView class
OrderMgmtView-showModel-01: className = OrderListView
.OrderListView-newInstance-01: root = an OrderMgmtView
 
 
Things are coming along nicely, good work.
 
--
Mark Miesfeld