From lechner Fri Dec 7 17:30:17 2001 From: Bob Lechner Subject: Re: bde2: BdeState project questions. - Ask your team to contribute. To: avu@cs.uml.edu (Anvi T. Vu) Date: Fri, 7 Dec 2001 17:29:50 -0500 (EST) Cc: 01f522 RJLRef: ~/01f522/bdeState_ProjectFdbk011207.txt This note replies to Anvi Vu's very pertinent comments about BdeState revisions to bde. It should be read by everyone involved with bde enhanceent projects, so we all anticipate the same refctoring of bde/src code. RJL ----------------------------------------------------- > From avu@cs.uml.edu Fri Dec 7 15:42:09 2001 > Date: Fri, 7 Dec 2001 15:42:08 -0500 (EST) > From: "Anvi T. Vu" > To: Bob Lechner > cc: Cris Fernandes , Ngoc TB Nguyen , > Suryamani Sastry , > Svitlana Grankovska , > "Anvi T. Vu" > Subject: Re: bde2: BdeState project questions. > > Hello, Professor! I look at the bde codes again and I have the > following thoughts. Please let me know if you agree with them: > > 1) I still think that a new data variable Widget stwd and a new function > set_stwd(stwd) should be added in to > BdeState class. The reason is that the variable Widget stwd inside the > Select class is updated every time Select::changestate(stwd, st) is > called. Widget stwd argument is passed from dostate() function in > smselect.cc to Select::changestate(stwd, st). In addition, dostate() is > used extensively in "leftmenu_cb.cc". > I believe you haven't analyzed the code enough - the only use of stwd in Select.cc is at the end of changestate: ---------- state = st; stwidget = stwd; --------------- The question whether stwidget should be encapsulated inside BdeState:: or not is independent of whether it should be set by changestate or by the CALLERS of changestate [or of its replacements setttyp, setoper and setstep (step is always set to 0 from left_menu_cb; step is changed only from changeAttribstate).] Further inspection (of where stwidget is actually used) shows that it is NEVER used at all!; so all your worry is unnecessaary - we can (I hope) just eliminate the variable stwidget! ByTheWay don't blame smselect author Brian Barry - he was constrained by time not to change the context of smselect functions. ------------ saturn.cs.uml.edu(54)> grep stwidget *.cc smselect.cc:488: stwidget = stwd; saturn.cs.uml.edu(55)> grep stwidget ../include/*.h ../include/graph.h:563: Widget stwidget; ../include/graph.h:567: stwidget = 0; saturn.cs.uml.edu(56)> pwd /usr/cs/fac1/lechner/fac5/bde2alpha_rl/bde/src -------------- > 2) If we want to remove smselect.cc, then we need to move the function > dostate() somewhere else. I think that we should put it in > "leftmenu_cb.cc" since this file use the function the most. > The non-class event-handling function dobuttonevent() in buttonevent.cc, dispatches every class method in *ops.cc. Dostate() needs to be encapsulated appropriately in BdeState:: to hide the EFFECTS of setttyp/oper/step on BdeState:: members. If (0,0,0) means stateName == graph__create_0 this is fine with me. Note that until ttype becomes a polymorphic class selector, BdeState probably needs a new 6-way enum of ttyp names to switch(ttyp) and a nested 6-way enum of oper names to switch(oper) and call the right *ops.cc method. There is ONE call to dostate in fileio.cc (which merely sets the initial default value of the OOMenu buttons - it should call a new BdeState function initState if 0,0,0 is not adequate. fileio.cc:1627: dostate(valuemenu, SCnode, NULL ); =================== Do we really need dostate? or statements to replace it? Looking inside dostate() the action is trivial (as I noted in a comment at the top): ------------------- // All that smselect really does is to select a cursor icon // shape that is appropriate for various states! - RJL 94/7/27 ... int currSt = selected.getstate(); // 'int' added by RJL - 2k0822 printf("dostate: selected.getstate = 0x%x = %d\n", currSt, currSt); switch (currSt) { //here there are so few changeCursor updates //that are not to the default arrow cursor shape. //that the unique cases shodl be mapped to their proper BdeState //methods and either done wihtin their state actions or // (probably simpler) done inside left_menu_cb when setttyp // and/or setoper are called.xa Of course the default cursser should be // reset first, so only these 5 exceptions need to be tested: //changestate (called fromleft_menu_cb) calls changeCanvasCurser // and pases the shape movehand for 3 link states, // passes addcross for one SArrow state, // and does one other from the top menu help button // (this one should also be moved back to its caller): case SHelp: helpfile("../man/diagram.help"); break; -------------------- dobuttonevent() ends with this comment (implying a pirate shape also, which I didn't find) and suggesting another place to revert to the default arrow shape: ------- /* make sure cursor is arrow. It could be pirate if last action was delete */ --------- > 3) I think we still need the state.h file in which the new state names > will replace the old ones. The reason is that state names are being > referenced in a lot of files. > I already answered this Q in my last message (below). I counted these refs and there aren't many outside of smselect and leftmenu*, both of which need to be rewritten or replaced. Most cases will be handled by renaming roughly 6 methods in *ops.cc to *_oper and redefining the case name labels in EVERY switch(state) to step0 [, step1 [, step2]] in switch(step). or if you prefer ttyp_oper_step#, Polymorphic functions will replace these later. > 4) Do you want to add printf statements into BdeState class so that it > will print the new state name every time a new state change happens (like > the function Select::changestate() and changeAtrribState)? > No, because pr_log.c will report BdeState::set* calls, by merely making BdeState a one-row table oin the bde schema. (I already proposed this in another note on BdeState tasks.) Logging is toggled on/off by fileio.cc calls to pr_startlog() and pr_stoplog. (Don't toggle often - the side effects are huge file writes.) > 5) Do you want us to keep the bdescv7.cpp (or bdeState.cpp) or should we > add the BdeState class definition into "graph.h" file and initilize > variable BdeState* bsp inside "bde.cc" (similar to how the variable Select > selected is initialized and how its class is defined)? The reason is that > "graph.h" is included in almost every file. > Do NOT merge this new Class in with other (class or non-class) code. The whole purpose of classes is encapsulation/separation of concerns. But DO split out bdeState.h definitions from bdeState.cc methods, and #include bdeState.h wherever its methods are called (clients need to be friends of BdeState or bsp needs to be public). > 6) If I am correct, you want us to do "switch(step)" inside all *ops.cc > files instead of the old "switch(state)"? Do we still need the variable > button_op? It's passed into most functions in *ops.cc files. What exactly > is it? I'm still confuse about it. > [?] We discussed event-driven GUIs like Xwindows and MSWindoes in class. Remember State Model (SM) architecture: there is a switch(state) (ie, step), then there is a nested switch(event) (i.e. buttonevent) inside each step case block. (see #4 above) > Thanks very much for your help! > You are welcome. It's a pleasure working with someone who is tring hard to get an in-depth understanding of bde source code. That's what reverse engineering is all about. I hope it helps you in future tasks on refactoring legacy C/C++ code into a better O-O design. > > Anvi > > > On Thu, 6 Dec 2001, Bob Lechner wrote: > > > > From avu@cs.uml.edu Wed Dec 5 21:12:03 2001 > > > Date: Wed, 5 Dec 2001 21:12:02 -0500 (EST) > > > From: "Anvi T. Vu" > > > To: Bob Lechner > > > cc: "Anvi T. Vu" > > > Subject: bde project questions.. > > > > > > Hello, Professor! For the project, we try to replace the Select > > > class by BdeState class. I notice that Select class contains a Widget stwd > > > variable. However, BdeState class doesn't have it. The Widget stwd is also > > > used inside changestate(Widget stwd, int st) function...I think that we > > > should add one more data variable, Widget stwd, into the BdeState class. > > > > I agree the widget argument must be handled some way (IF it is really used > > or even passed on or through the module.. I can't read the code right now. > > > > But pay attention to the function being done, and don't try to > > make BdeState do too many unrelated things. > > > > > We will also need to add one more member function, > > > set_StateCode(Widget stwd, uint sc) into BdeState class to subtitute for > > > the changestate (Widget stwd, int st) in Select class. > > > > Why? Look at left_menu_cb first so your change is compatible with it. > > Left_menu_cb is (I think) another place where selected.changestate is used. > > Use grep -n state bde/srci/*.cc | wc to find out how many references and > > which files. I don't think there are too many to convert but > > you need to find them all to share the workload equally. > > > > By the way, Chris and the others (5 total) elected to change > > ALL the *ops.cc files (and the others) so Select:: can be replaced. > > > > grep in bde/src shows 442 lines containing 'state'. > > > > This counts refs in each file: > > --------------------------------------------- > > saturn.cs.uml.edu(623)> grep state *.cc|wc > > 422 3454 29555 > > saturn.cs.uml.edu(624)> foreach f (ls *.cc) > > foreach? echo $f > > foreach? grep state $f | wc > > foreach? end > > -------------------------------------------- > > Results are in ~/01f522/bdeRefs2State.011206. > > > > The distribution is very unequal : > > buttonevent.cc has 33 refs; > > left_menu_cb has 67, smselect has 82. > > *ops cc have 157. > > These nine files total 339 refs. > > > > > > > Please let us know what you think. > > > Also, you said that if we replace Select class by BdeState class > > > then we don't need state.h file anymore. I can't figure out how we can do > > > that because the state names are used in many files, which doesn't include > > > *ops.cc. If we don't have a state.h file that contains all state names and > > > their values, how can we make those files compile correctly? > > > Anvi > > > > > > > >