Issue 3422044: Wizard Introduction Documentation
DescriptionRoundup issue:
http://bugs.tryton.org/roundup/issue1900
Patch Set 1 #
Total comments: 21
Patch Set 2 : Updated text in response to comments. #Patch Set 3 : Updated for 1.8 and fixed typos. #
Total comments: 35
Patch Set 4 : Updates in response to comments. #
Total comments: 2
Patch Set 5 : Updated to latest trunk. #Patch Set 6 : The right diff. #Patch Set 7 : Was missing link in topics. #
Total comments: 3
Patch Set 8 : Use note directive. Change feature header to overview. #
Total comments: 81
Patch Set 9 : Updated with respect to comments. #
Total comments: 3
MessagesTotal messages: 20
http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:10: A wizard transitions a user to and from a set of states. When states change I think it is good to define it as a finite-state machine: http://en.wikipedia.org/wiki/Finite_state_machine http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:14: actions and/or used to generate reports. or modify records http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:31: * A wizard can print reports which are subclasses of Report. This is not really the wizard that print reports. But the wizard fire on the client-side, the print of a report and in a more generic way any Actions (ir.action) http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:35: * A wizard can be attached to a button within a view. In fact, a wizard is an ir.action also. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:37: * A wizard can be associated with a model so that the wizard receives that It is define on the ir.action.wizard. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:44: Wizards can be thought of as a loop. The loop starts with the special init I don't think `loop` is the right term. Finite-state Machine is better. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:46: the state's definition is retrived. If the state's definition defines an retrieved http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:69: The end state is the end of the wizard and it will stop executing. The end state is a virtual state, there is no definition of it. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:85: # prior states and definitions There is no order http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:115: def actions_entry(self, cursor, user, action_input, context=None): There is no more cursor, user nor context http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:154: If an ir action definition is returned the client will execute that action. ir.action http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:159: another document? The same result as a read on a ir.action.* record. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:202: in Dialog is really hard to read. Yes, the one with True will be the default button http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:250: def get_next_state(self, cursor, user, data, context=None): no more cursor, user, context http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:260: loop repeatedly back to a state until as many reports are printed as desired. This is not so common. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:264: TODO: Is this description of get_id_from_action always correct? It looks but this should be a remark as it is rarely used. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:271: The state print_next will be the next state after printing is finished. next_print
Sign in to reply to this message.
Responded to comments with new patch and some replies. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:31: * A wizard can print reports which are subclasses of Report. On 2010/12/07 22:46:25, ced wrote: > This is not really the wizard that print reports. > But the wizard fire on the client-side, the print of a report and in a more > generic way any Actions (ir.action) So is the print result type just a shortcut for the more generic action result type ? http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:35: * A wizard can be attached to a button within a view. On 2010/12/07 22:46:25, ced wrote: > In fact, a wizard is an ir.action also. Okay I tried to rephrase this but I'm not sure if its correct. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:85: # prior states and definitions On 2010/12/07 22:46:25, ced wrote: > There is no order Yeah I wanted to convey that more states would occur before or after but I have changed the comment text so it won't mislead users into thinking the order of the dictionary keys is of any importance. http://codereview.appspot.com/3422044/diff/1/doc/topics/wizards/index.rst#new... doc/topics/wizards/index.rst:260: loop repeatedly back to a state until as many reports are printed as desired. On 2010/12/07 22:46:25, ced wrote: > This is not so common. Okay I mention this because it was a problem for me when I tried to print two copies of invoice after making a sale. I will remove the suggestion that it is common though.
Sign in to reply to this message.
Very great that you start the wizard documentation! Thanks a lot. I found no bigger issues, but have some comments in place. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:16: Basics and features: Features Overview ... 'Basic' is duplicated the section headings http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:44: definition. Once this result is fulfilled the wizard transitions to a new If the state's ... then each action is executed. Then a result... unclear for me. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:51: =================================== ...field -.- You not used a period in former headings. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:61: The init state is the starting state of the wizard. The wizard is ... of all wizards. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:65: state is virtual and does not have a definition. A state with name 'end' is used by convention, to stop a wizard. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:85: Note that the order of the entries in the states field has no signifigance. ... no significance for the succession of evaluation. For readability, they should be ordered in the sequence of evaluation. ...or s/t like this http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:89: dictionary that must contain an entry for the type of the state. The key is 'type' #87 - #89: width > 80 columns ? http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:97: actions. The key should be 'actions'. The value is a list of strings which are is _an ordered_ list of strings http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:103: result dictionary. After all actions are executed the actions result dictionary After all actions _of a state_ are executed... http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:110: def actions_entry(self, action_input): Aren't actions in the actions list more general just methods? These methods _could_ be methods of ir.action object? Is action_input def actions_entry(self, *arg, **kwarg) where arg and kwarg are evaluated by the given method? http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:132: The STATE_RESULT_TYPE is a string and must be one of the following: 'action', ... following keywords: http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:139: type -- action action type ? http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:147: The method in this example is 'action_name'. ...in the following code example... http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:156: After the ir action has been resolved the wizard will progress to the state is 'resolved' the right term? I have 'proceeded' in mind, but I am not native English speaker. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:182: type -- form form type ? http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:192: transition the the wizard to. The second slot is the label of the button. The transition --the-- the... http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:193: third slot is the the icon to use. If the definition is a 4-tuple than the slot is --the-- the... http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:197: TODO: What inputs can be passed to affect the form defaults? You can return a context with some data from the current state to the next state, which is type form. There you can provide default_<field name> methods, which evaluate the context. There is one open talk about this: https://bugs.tryton.org/roundup/issue1212 http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:215: type -- choice choice type ? http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:245: type -- print print type? http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:256: of the last state. 'string key ids' is unclear. Why not only 'keyword `ids`' or 'key `ids`'?
Sign in to reply to this message.
Updates in response to comments. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:44: definition. Once this result is fulfilled the wizard transitions to a new On 2010/12/08 09:06:58, udono wrote: > If the state's ... then each action is executed. Then a result... > > unclear for me. Yes it is confusing. I have removed this detail and hope that it can be explained later. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:51: =================================== On 2010/12/08 09:06:58, udono wrote: > ...field -.- > > You not used a period in former headings. fixed http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:61: The init state is the starting state of the wizard. The wizard is On 2010/12/08 09:06:58, udono wrote: > ... of all wizards. fixed http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:65: state is virtual and does not have a definition. On 2010/12/08 09:06:58, udono wrote: > A state with name 'end' is used by convention, to stop a wizard. I changed phrasing to match the init phrasing. The end state is not just convention though because the tryton interprets the end state in a special way. A convention is something like how many spaces to indent and is not enforced. If your wizard never transitions to the end state it will never stop therefore it is not just a convention. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:85: Note that the order of the entries in the states field has no signifigance. On 2010/12/08 09:06:58, udono wrote: > ... no significance for the succession of evaluation. For readability, they > should be ordered in the sequence of evaluation. > ...or s/t like this This is convention though and I updated the text. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:89: dictionary that must contain an entry for the type of the state. The key is 'type' On 2010/12/08 09:06:58, udono wrote: > #87 - #89: width > 80 columns ? fixed http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:97: actions. The key should be 'actions'. The value is a list of strings which are On 2010/12/08 09:06:58, udono wrote: > is _an ordered_ list of strings fixed the wording http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:110: def actions_entry(self, action_input): On 2010/12/08 09:06:58, udono wrote: > Aren't actions in the actions list more general just methods? > These methods _could_ be methods of ir.action object? > Is action_input def actions_entry(self, *arg, **kwarg) where arg and kwarg are > evaluated by the given method? I don't think that is possible, it must be a method of the wizard that takes only one argument. These "actions" really should not be called actions at all but rather "client result customizers" since they construct a dictionary that will be passed to whatever the client does with the result, ie. a form, an action or printing. This is really hard to explain with the given terms. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:132: The STATE_RESULT_TYPE is a string and must be one of the following: 'action', On 2010/12/08 09:06:58, udono wrote: > ... following keywords: I updated this but I think the word "keyword" might be misleading. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:139: type -- action On 2010/12/08 09:06:58, udono wrote: > action type > ? I think putting the word "type" first is more clear because there may be some meaning to action type, form type, print type, and of course state type. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:147: The method in this example is 'action_name'. On 2010/12/08 09:06:58, udono wrote: > ...in the following code example... fixed http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:156: After the ir action has been resolved the wizard will progress to the state On 2010/12/08 09:06:58, udono wrote: > is 'resolved' the right term? I have 'proceeded' in mind, but I am not native > English speaker. I changed to 'performed'. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:197: TODO: What inputs can be passed to affect the form defaults? On 2010/12/08 09:06:58, udono wrote: > You can return a context with some data from the current state to the next > state, which is type form. There you can provide default_<field name> methods, > which evaluate the context. There is one open talk about this: > https://bugs.tryton.org/roundup/issue1212 This isn't the usual context, you mean the 'datas' variable right? I am thinking about referring to it as the result input. How do default_<field name> methods access this ? It seems that the datas['form'] dictionary just overrides whatever defaults that the default_ methods return. http://codereview.appspot.com/3422044/diff/7001/doc/topics/wizards/index.rst#... doc/topics/wizards/index.rst:256: of the last state. On 2010/12/08 09:06:58, udono wrote: > 'string key ids' is unclear. Why not only 'keyword `ids`' or 'key `ids`'? I tried to remove excess usages of the word 'string'.
Sign in to reply to this message.
LGTM http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:10: A wizard is essentially a finite state machine. When states change maybe links to explain wizard and finit state machine more general? .. _wp_wizard:http://en.wikipedia.org/wiki/Wizard_%28software%29 .. _wp_finite_state_machine:http://en.wikipedia.org/wiki/Finite-state_machine
Sign in to reply to this message.
On 2011/05/24 12:55:14, udono wrote: > LGTM > > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst > File doc/topics/wizards/index.rst (right): > > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst... > doc/topics/wizards/index.rst:10: A wizard is essentially a finite state machine. > When states change > maybe links to explain wizard and finit state machine more general? > .. _wp_wizard:http://en.wikipedia.org/wiki/Wizard_%28software%29 > .. _wp_finite_state_machine:http://en.wikipedia.org/wiki/Finite-state_machine I still find it to verbose which will make it difficult to maintain.
Sign in to reply to this message.
http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:10: A wizard is essentially a finite state machine. When states change On 2011/05/24 12:55:15, udono wrote: > maybe links to explain wizard and finit state machine more general? > .. _wp_wizard:http://en.wikipedia.org/wiki/Wizard_%28software%29 > .. _wp_finite_state_machine:http://en.wikipedia.org/wiki/Finite-state_machine We must assume that the reader is familiar with general development concepts already: finite state machine, class, subclass, server side, client side, etc. Otherwise the documentation will grow to large.
Sign in to reply to this message.
On 2011/05/24 14:02:51, ced wrote: > On 2011/05/24 12:55:14, udono wrote: > > LGTM > > > > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst > > File doc/topics/wizards/index.rst (right): > > > > > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst... > > doc/topics/wizards/index.rst:10: A wizard is essentially a finite state > machine. > > When states change > > maybe links to explain wizard and finit state machine more general? > > .. _wp_wizard:http://en.wikipedia.org/wiki/Wizard_%28software%29 > > .. _wp_finite_state_machine:http://en.wikipedia.org/wiki/Finite-state_machine > > I still find it to verbose which will make it difficult to maintain. The proposed links are too verbose or the wizard introduction documentation is too verbose?
Sign in to reply to this message.
> The proposed links are too verbose or the wizard introduction > documentation is too verbose? The wizard introduction. -- Cédric Krier B2CK SPRL Rue de Rotterdam, 4 4000 Liège Belgium Tel: +32 472 54 46 59 Email/Jabber: cedric.krier@b2ck.com Website: http://www.b2ck.com/
Sign in to reply to this message.
On 2011/05/25 22:38:27, ced wrote: > > The proposed links are too verbose or the wizard introduction > > documentation is too verbose? > > The wizard introduction. > > -- > Cédric Krier > > B2CK SPRL > Rue de Rotterdam, 4 > 4000 Liège > Belgium > Tel: +32 472 54 46 59 > Email/Jabber: mailto:cedric.krier@b2ck.com > Website: http://www.b2ck.com/ Well if you can be more specific I can refine it.
Sign in to reply to this message.
For me the text in general is good. There could be some parts which can be shortend, but it can be carefully done in a later step. For someone who needs an introduction into wizards it is definitely the best document we have and worth reading. I do not understand why it takes such a long time to get a documentation part which is neither error prone nor unreadable into Tryton. Please review the text and put concrete proposals in place. Or even better, put the doc into trytond documentation, and let us discuss about shortenings later. http://codereview.appspot.com/3422044/diff/28001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/28001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:16: Features Maybe better section name: Overview http://codereview.appspot.com/3422044/diff/28001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:257: Note: An optional entry exists with key 'get_id_from_action'. .. note:: http://codereview.appspot.com/3422044/diff/28001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:263: NOTE: Only one report can be printed at once. In order to print multiple times .. note::
Sign in to reply to this message.
On 2011/05/25 22:28:40, Ian Wilson wrote: > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst > File doc/topics/wizards/index.rst (right): > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst... > doc/topics/wizards/index.rst:10: A wizard is essentially a finite state machine. > When states change > On 2011/05/24 12:55:15, udono wrote: > > maybe links to explain wizard and finit state machine more general? > > .. _wp_wizard:http://en.wikipedia.org/wiki/Wizard_%28software%29 > > .. _wp_finite_state_machine:http://en.wikipedia.org/wiki/Finite-state_machine > We must assume that the reader is familiar with general development concepts > already: finite state machine, class, subclass, server side, client side, etc. As a reader of your text, I tell: before reading I had no idea about a finite state machine and no deep knowledge about general wizard conception in software engineering... For the terms class, subclass, server side, client side, your assumption are right for this document. > Otherwise the documentation will grow to large. It is just some links as mind refresh to a stipulative definition. A link usually takes no additional space. Without links or definitions it is just dropping names.
Sign in to reply to this message.
On 2011/05/31 07:52:28, udono wrote: > On 2011/05/25 22:28:40, Ian Wilson wrote: > > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst > > File doc/topics/wizards/index.rst (right): > > > http://codereview.appspot.com/3422044/diff/13001/doc/topics/wizards/index.rst... > > doc/topics/wizards/index.rst:10: A wizard is essentially a finite state > machine. > > When states change > > On 2011/05/24 12:55:15, udono wrote: > > > maybe links to explain wizard and finit state machine more general? > > > .. _wp_wizard:http://en.wikipedia.org/wiki/Wizard_%28software%29 > > > .. > _wp_finite_state_machine:http://en.wikipedia.org/wiki/Finite-state_machine > > We must assume that the reader is familiar with general development concepts > > already: finite state machine, class, subclass, server side, client side, etc. > > > As a reader of your text, I tell: before reading I had no idea about a finite > state machine and no deep knowledge about general wizard conception in software > engineering... > > For the terms class, subclass, server side, client side, your assumption are > right for this document. > > > Otherwise the documentation will grow to large. > It is just some links as mind refresh to a stipulative definition. A link > usually takes no additional space. Without links or definitions it is just > dropping names. Yeah I sort of agree about adding the links but I think we should see if it comes up on mailing list or IRC. For example people coming to IRC and asking about wizards or finite state machines. We can add links then. I integrated your other suggestions though: note directives and features -> overview.
Sign in to reply to this message.
I think I'm not enthusiast about this documentation because I think there is a lot of flaw in the wizard design inherited from OpenERP. I don't know if it is good to keep to try to write the doc now or is it not better to first fix the wizard design and write the doc during this refactoring. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:16: Overview The basics: (like in models) http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:23: wizard can make. The `states` field defines each state of the wizard: * (defines what compose a state) http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:36: Control flow Execution http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:39: As stated earlier a wizard is essentially a finite state machine. The wizard I don't think it is required to repeat this. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:41: each transition to a new state the state's definition is retrieved. The state I don't see what this means in code. There is no real transition definition. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:43: wizard transitions to a new state. The wizard goes to the next state once the user click on one button. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:45: The way in which the result is generated and fulfilled depends on the state's I don't understand. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:49: States and the states field Simply: States http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:52: A wizard must contain a states field that defines the possible states it can Already defines above http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:55: Special states I would call them: reserved states http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:61: instantiated before assuming this state. No wizard is instantiated http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:64: executing upon transitioning to the end state. The end state is virtual Second sentence repeat the first one. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:67: States field Just states http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:70: The states field is a dictionary. Each entry represents a state definition. Already defines above. Better to just focus here on a state. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:73: transition to the state. This dictionary is referred to as the Why transition? It is the definition of the state. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:78: .. code-block:: python I think it is better to show a complete state definition http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:88: as a convention the states should be roughly ordered by evaluation. This is not a convention. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:107: result dictionary is placed in the dictionary under the 'datas' key. I think this paragraph can be simplified by saying the state of the `machine` is stored in 'datas' and actions update it. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:108: I think this 3 paragraphs will be simpler and more understanding with one example before text and just definition of each parts. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:139: State result types State results http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:142: type -- action Just: action http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:149: of a the method must be either an empty dictionary or an `ir.action` definition. the result must be an `ir.action` definition http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:153: For example a developer might want the client to execute an `ir.action` to open I don't think the example here is useful. Better to link to a future `ir.action` documentation. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:159: After the `ir.action` has been performed the wizard will progress to the state False, the action is run in parallel than the wizard. So the wizard goes directly to the next state and does't wait for the end of the new `ir.action` http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:165: .. code-block:: python Better to put the example firt. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:178: The action method must have this structure: structue -> signature http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:190: The 'object' key defines the model to use for the form. The `object` values is the name of the model for the form. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:193: transition to its specified state. Each button definition should be Each button defines the possible next state the user must choice. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:194: a 3-tuple or 4-tuple. The first slot is the state that the button will Better to have an example: (<state name>, <label>, <icon name>[, <boolean>]) http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:205: .. code-block:: python Again example first is better http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:224: This type allows the next state to be determined programmatically could remove "programmatically" in the doc all is programmatic :-) http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:230: dictionary or the special state end. The method define in 'next_state' will return the name of the next state. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:233: Example first http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:255: This type will print a report. This is a shortcut state for an action state that will return the report definition. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:276: Example first http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:312: Defining a wizard class No need of this second title http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:315: In this example we define a very simple wizard that completes the lines in remove "very simple"
Sign in to reply to this message.
On 2011/06/02 12:53:52, ced wrote: > I think I'm not enthusiast about this documentation because I think there is a > lot of flaw in the wizard design inherited from OpenERP. > I don't know if it is good to keep to try to write the doc now or is it not > better to first fix the wizard design and write the doc during this refactoring. Agree. For me the complexity of this wizard documentation shows in an obvious way that the wizard implementation in Tryton seems not clean and common. So much explanation for a should-be small topic... But anyway this documentation gives us all good overview about the topic for now. It is a good introduction for the comming discussion about the new wizard interface. Because we can show how it is for now and how it should be in the new implementation. But in the sake of transparence for new developer, this document should be integrated, when it is overworked.
Sign in to reply to this message.
http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:16: Overview On 2011/06/02 12:53:53, ced wrote: > The basics: > > (like in models) I found the page you are referring too and I do not think this applies here. This section is meant to be an overview of wizards whereas "The basics" in models really is just the basics. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:23: wizard can make. On 2011/06/02 12:53:53, ced wrote: > The `states` field defines each state of the wizard: > > * (defines what compose a state) Wasn't clear on your wording but updated text to what I thought your intentions were. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:36: Control flow On 2011/06/02 12:53:53, ced wrote: > Execution I don't think execution takes into account that often state transitions do not occur until a user hits a button. I did not change this. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:39: As stated earlier a wizard is essentially a finite state machine. The wizard On 2011/06/02 12:53:53, ced wrote: > I don't think it is required to repeat this. It is not required but I think it makes reading much easier. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:41: each transition to a new state the state's definition is retrieved. The state On 2011/06/02 12:53:53, ced wrote: > I don't see what this means in code. > There is no real transition definition. This is how I interpreted the code: # execute can mean many things and depends on composition/definition of state result = execute(state definition in states field for new state) send result to client wizard in client now shows new state http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:43: wizard transitions to a new state. On 2011/06/02 12:53:53, ced wrote: > The wizard goes to the next state once the user click on one button. The next state might not always depend on button clicking. For example in order to print more than one document the wizard loops between states printing a document at a time until the documents are depleted. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:45: The way in which the result is generated and fulfilled depends on the state's On 2011/06/02 12:53:53, ced wrote: > I don't understand. What the wizard does for a state depends on if the state is of type form, choice, print, etc. Is this wording too confusing? http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:49: States and the states field On 2011/06/02 12:53:53, ced wrote: > Simply: > > States Changed. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:52: A wizard must contain a states field that defines the possible states it can On 2011/06/02 12:53:53, ced wrote: > Already defines above The first section is meant as a summary. This section is meant to be more comprehensive that is why it is mentioned again. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:55: Special states On 2011/06/02 12:53:53, ced wrote: > I would call them: reserved states I associate the word "reserved" with something that cannot be used. I changed it anyways. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:61: instantiated before assuming this state. On 2011/06/02 12:53:53, ced wrote: > No wizard is instantiated Changed to initialized. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:64: executing upon transitioning to the end state. The end state is virtual On 2011/06/02 12:53:53, ced wrote: > Second sentence repeat the first one. Fixed. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:67: States field On 2011/06/02 12:53:53, ced wrote: > Just states Repeats the parent heading. I think fields makes it clear this applies to the attribute on the subclass. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:70: The states field is a dictionary. Each entry represents a state definition. On 2011/06/02 12:53:53, ced wrote: > Already defines above. > Better to just focus here on a state. This section is meant to give a detailed description of what the states field actually is. That is not done elsewhere. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:73: transition to the state. This dictionary is referred to as the On 2011/06/02 12:53:53, ced wrote: > Why transition? It is the definition of the state. Yes, it does not quite adhere to a finite state machine. I guess the transition would be what is executed between the old state and arriving at a new state. It seems that a state's definition really defines a transition but I changed it anyways. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:78: .. code-block:: python On 2011/06/02 12:53:53, ced wrote: > I think it is better to show a complete state definition I try to break it up because the key names are so confusing. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:88: as a convention the states should be roughly ordered by evaluation. On 2011/06/02 12:53:53, ced wrote: > This is not a convention. Removed. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:107: result dictionary is placed in the dictionary under the 'datas' key. On 2011/06/02 12:53:53, ced wrote: > I think this paragraph can be simplified by saying the state of the `machine` is > stored in 'datas' and actions update it. The names datas and actions are so confusing that is why there is so much explicitness here. Once those names are removed this paragraph can be simplified. I would like to talk more about a new mental model and nomenclature but think we should finish this first. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:108: On 2011/06/02 12:53:53, ced wrote: > I think this 3 paragraphs will be simpler and more understanding with one > example before text and just definition of each parts. I read many examples trying to get wizards to work and I found that not to be the case. The nomenclature and flow is so confusing that it must be spelled out explicitly until the better nomenclature and mental model is developed. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:139: State result types On 2011/06/02 12:53:53, ced wrote: > State results This section is about the state result types not about state results. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:142: type -- action On 2011/06/02 12:53:53, ced wrote: > Just: action Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:149: of a the method must be either an empty dictionary or an `ir.action` definition. On 2011/06/02 12:53:53, ced wrote: > the result must be an `ir.action` definition I think this is confusing because you can return an empty dictionary which will be ignored or a definition which will be used. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:153: For example a developer might want the client to execute an `ir.action` to open On 2011/06/02 12:53:53, ced wrote: > I don't think the example here is useful. > Better to link to a future `ir.action` documentation. Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:159: After the `ir.action` has been performed the wizard will progress to the state On 2011/06/02 12:53:53, ced wrote: > False, the action is run in parallel than the wizard. > So the wizard goes directly to the next state and does't wait for the end of the > new `ir.action` Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:165: .. code-block:: python On 2011/06/02 12:53:53, ced wrote: > Better to put the example firt. Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:178: The action method must have this structure: On 2011/06/02 12:53:53, ced wrote: > structue -> signature Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:190: The 'object' key defines the model to use for the form. On 2011/06/02 12:53:53, ced wrote: > The `object` values is the name of the model for the form. Somewhat adjusted. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:193: transition to its specified state. Each button definition should be On 2011/06/02 12:53:53, ced wrote: > Each button defines the possible next state the user must choice. Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:194: a 3-tuple or 4-tuple. The first slot is the state that the button will On 2011/06/02 12:53:53, ced wrote: > Better to have an example: > > (<state name>, <label>, <icon name>[, <boolean>]) Both versions are represented in the included example. I just made a note of that. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:205: .. code-block:: python On 2011/06/02 12:53:53, ced wrote: > Again example first is better Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:224: This type allows the next state to be determined programmatically On 2011/06/02 12:53:53, ced wrote: > could remove "programmatically" in the doc all is programmatic :-) You are right. I removed programmatic. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:230: dictionary or the special state end. On 2011/06/02 12:53:53, ced wrote: > The method define in 'next_state' will return the name of the next state. Simplified. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:233: On 2011/06/02 12:53:53, ced wrote: > Example first Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:255: This type will print a report. On 2011/06/02 12:53:53, ced wrote: > This is a shortcut state for an action state that will return the report > definition. I don't understand this. Does it not print? http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:276: On 2011/06/02 12:53:53, ced wrote: > Example first Done. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:312: Defining a wizard class On 2011/06/02 12:53:53, ced wrote: > No need of this second title Well I was hoping more examples would be added. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:315: In this example we define a very simple wizard that completes the lines in On 2011/06/02 12:53:53, ced wrote: > remove "very simple" Done.
Sign in to reply to this message.
I stop the review here. I think we spend too much time to document the poor designed Wizard. I will personally spend my time on designed a better Wizard. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:36: Control flow On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > Execution > I don't think execution takes into account that often state transitions do not > occur until a user hits a button. I did not change this. If you keep the state machine comparison then you must talk about execution of this state machine. I don't see what "hitting button" has to do here. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:39: As stated earlier a wizard is essentially a finite state machine. The wizard On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > I don't think it is required to repeat this. > It is not required but I think it makes reading much easier. Repeating makes the management of the documentation too complicate. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:41: each transition to a new state the state's definition is retrieved. The state On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > I don't see what this means in code. > > There is no real transition definition. > > This is how I interpreted the code: We don't need of a personal interpretation but only fact and description. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:45: The way in which the result is generated and fulfilled depends on the state's On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > I don't understand. > > What the wizard does for a state depends on if the state is of type form, > choice, print, etc. Is this wording too confusing? What I don't understand is what is the result generated? I really don't think you must describe the internal behavior of the execution of the wizard. The developer doesn't need it. It only requires to understand the finite state machine and the type of possible states and how they work. The way how the communication with the client works is not relevant because it could change in the future, it is not linked to just the GTK client (see proteus). The documentation must talk about concept not describe the code if the developer wants to have internal knowledge of it he just has to read the code. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:52: A wizard must contain a states field that defines the possible states it can On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > Already defines above > > The first section is meant as a summary. This section is meant to be more > comprehensive that is why it is mentioned again. So again repeating makes the management more complicated. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:67: States field On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > Just states > Repeats the parent heading. I think fields makes it clear this applies to the > attribute on the subclass. fields could be mixed with the fields of a Model so better to avoid it. http://codereview.appspot.com/3422044/diff/30001/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:70: The states field is a dictionary. Each entry represents a state definition. On 2011/06/07 20:51:37, Ian Wilson wrote: > On 2011/06/02 12:53:53, ced wrote: > > Already defines above. > > Better to just focus here on a state. > This section is meant to give a detailed description of what the states field > actually is. That is not done elsewhere. already define line 22 http://codereview.appspot.com/3422044/diff/33002/doc/topics/wizards/index.rst File doc/topics/wizards/index.rst (right): http://codereview.appspot.com/3422044/diff/33002/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:23: transitions of each state. I don't understand. `states` fields just define the states. A state is not composed and there is no transition definition but just buttons. http://codereview.appspot.com/3422044/diff/33002/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:61: initialized before assuming this state. A wizard is not initialized. It is just a session that is created but again this is internal stuff that should not be in the documentation. http://codereview.appspot.com/3422044/diff/33002/doc/topics/wizards/index.rst... doc/topics/wizards/index.rst:72: is referred to as the state definition dictionary. the state definition dictionary -> the state
Sign in to reply to this message.
|
