Community
Participate
Working Groups
This thread: http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg19674.html Problem is class cannot be extended (meaningfully). Steps cannot be extended or re-used. Should be re-design to satisfy the following constrains: User should be able to a) add new steps before/after a specific FLS step b) remove/disable a step c) change a step d) call call default step methods from his class e) re-order steps Additionally would be nice to be able to fix actual class if we need to in maintenance release
Created attachment 175366 [details] patch using reflection patch that contains a solution to all problems described above. a) add new method in user class which follows naming convention step_##{_##}*_<step_name> steps would be sorted alpha-numberically before loading. It is always possible to instert a step between two other steps b) remove/disable a step override a step and do nothing c) change a step override a step d) call call default step methods from his class call call super.<...> from overriden method or even from other methods e) re-order steps override initializeSteps, call super, then user can find a step index using findStepIndex method and re-order in array. Override getSteps to return new array
Trying to wove discussion into the bug: > Marc Khouzam wrote: > > If you look at the step in FLS which specifies a core file, you can > see that there is a subclass defined right in the extension of the Step. > It just keeps that code localized to where it is needed. You can look at my patch. It does define a class but it is not a "Step" class. > >> > If I understand correctly, the added value of this suggestion >> > is the automatic sorting by method name. >> > As Mikhail pointed out, re-ordering is then a problem. >> Not really. After sequence is created it can be re-arranged >> in array itself. > > But then what is the point of having indexes in the names? So you don't have to do it manually in a fist place. The other list of "named" steps, and you can make a mistake by using different name then a method, and it is harder to refactor. > >> Also added value is a big decrease in a code size you have to write >> which significantly increase readability and maintainability > > Allowing to properly extend FLS would give this better > readability and maintainability, however, I'm trying to figure out > the difference/value between your solution and the other ones that > have been proposed. Example? One of them was to create a kind of factory methods for each step which returns step class. Minus of this would be - if you override this factory method you cannot really call "super", you have to re-implement the whole thing. The only way to call it to get Step and call excecute directly - but it will end your monitor. And another minus you have to create another class instance every time and this is very ugly code. Plus would be you can define full step if you need more than execute method. Ability to put "members" in a step class I don't count as plus, as I said I don't see any difference between putting them in Step class or any other class. Actually advantage of creating another non-anonymous class is other people can use it too (extend, etc). > >> > For ordering I would hard-code a number in the name, but >> > I would create an array that lists all the methods >> > in the right order. This can be overridden more easily. >> > But is still not very future-proof for extending classes. >> >> That is what initialize method is doing, see below > > Sorry, my sentence should have read "I would _not_ hard-code > a number in the name". What is the value of that number, if > the extending class can modify the array anyway? To automatically calculate initial order. Full proof. >> >> Depends. We only have problem with maintenance build, this is a >> solution. > > Why do we only have a problem for the maintenance build? build==release Because there is no problem adding protected methods in minor or major release. > >> Solution for head should be always changing parent class. >> Do we have a bug for that? I post a patch and we can discuss it better >> when it is all written. > > Thanks. > Let's continue this in the bug. >
(In reply to comment #1) > Created an attachment (id=175366) > patch using reflection > > patch that contains a solution to all problems described above. > > a) add new method in user class which follows naming convention > step_##{_##}*_<step_name> > steps would be sorted alpha-numberically before loading. > It is always possible to instert a step between two other steps We will probably end-up with conflicts here. Say FLS has step_01_doThis() and step_02_doThat(). An overriding class would create step_01_01_doSomethingElse(). That all works. But then, we decide to add a step to FLS between 1 and 2, we would create step_01_01_doThatOtherThing(). The order of step_01_01_doThatOtherThing() and step_01_01_doSomethingElse() become 'random', based on what name happened to be chosen after the naming scheme pattern. This would only happen rarely, but still seems not very precise to me. If I understand correctly, putting ## in the method names allows to keep the methods ordered. Also, using reflection allows to add steps from any base or overriding class and it will be picked up; I assume that is why you use reflection. Instead though, why not use Pawel and Mikhail's suggestion of Step.getId(), which could also be sorted alpha-numerically. That way, the name would not be an API. As for adding a new step, the base class should have an includeStep() method that would be used by overriding classes to add a new step, and its getId() method would deal with its position. > b) remove/disable a step > override a step and do nothing Good. > c) change a step > override a step Good. > d) call call default step methods from his class > call call super.<...> from overriden method or even from other methods Good > e) re-order steps > override initializeSteps, call super, then user can find a step index using > findStepIndex method and re-order in array. Override getSteps to return new array Seems ok, but I think that will be non-trivial to track the order of steps. But not a show-stopper. Please turn on API tooling for patches. Why not complete the rm in each step, instead of returning a boolean to indicate if the rm should be completed after execute() This adds more complexity to an already complex DSF requirement. About overriding entire Steps instead of execute(), if we look at step_15_setRemoteTargetConnection(), it only allows to replace the entire step, instead of overriding it. We could make it overridable by making the class RemoteTargetConnection protected. I personally prefer to keep the code relevant to a step inside that step. I think we can override the full steps, but still use your naming scheme for the step name. But let's leave this for later. Have you used this patch to create your own overriding of FLS? I think we should try it before choosing a final solution, to make sure we can override as we expect.
I don't really have a strong opinion one way or the other on Alena's proposed solution, but one option to consider is to use java 5 annotation instead of method names for the step ordering. Annotations can be accessed through reflections APIs but they at least have some compiler checking support.
Also, if you come to a consensus on this let's also consider generalizing the solution to the Sequence class itself. As this discussion illustrates the current Sequence class is somewhat awkward to use and extend.
(In reply to comment #0) > This thread: > http://dev.eclipse.org/mhonarc/lists/cdt-dev/msg19674.html > > Problem is class cannot be extended (meaningfully). > Steps cannot be extended or re-used. Should be re-design to satisfy the > following constrains: > > User should be able to > a) add new steps before/after a specific FLS step > b) remove/disable a step > c) change a step > d) call call default step methods from his class > e) re-order steps > > Additionally would be nice to be able to fix actual class if we need > to in maintenance release We can argue what is or is not "meaningful". But the following is a simple way to satisfy a)-e) with the existing code. Say you want to use the default steps with indexes 0, 2 and 5 as the steps 1,2 and 3 in your sequence and insert a custom step at position 0. You need to do the following in the class that extends FinalLaunchSequence: private Step[] fMySteps; @Override public Step[] getSteps() { if ( fMySteps == null ) { Step[] defaultSteps = super.getSteps(); fMySteps = new Step[4]; fMySteps[0] = new YourCustomStep(); fMySteps[1] = defaultSteps[0]; fMySteps[2] = defaultSteps[2]; fMySteps[3] = defaultSteps[5]; } return fMySteps; } I agree, this code is a hack and not "user friendly". Having a repository of steps accessible by names or by ids would be enough. I don't think the idea of having indexes hard-coded into the step names to get compile time errors when the order of the original class changes is a good one. Note that you didn't include it into your requirements either.
> > We will probably end-up with conflicts here. Say FLS has step_01_doThis() and > step_02_doThat(). > An overriding class would create step_01_01_doSomethingElse(). That all works. > But then, we decide to add a step to FLS between 1 and 2, we would create > step_01_01_doThatOtherThing(). > > The order of step_01_01_doThatOtherThing() and step_01_01_doSomethingElse() > become > 'random', based on what name happened to be chosen after the naming scheme > pattern. > > This would only happen rarely, but still seems not very precise to me. > > If I understand correctly, putting ## in the method names allows to keep the > methods ordered. Also, using reflection allows to add steps from any base or > overriding class and it will be picked up; I assume that is why you use > reflection. Instead though, why not use Pawel and Mikhail's suggestion of > Step.getId(), which could also be sorted alpha-numerically. That way, the name > would not be an API. As for adding a new step, the base class should have an > includeStep() method that would be used by overriding classes to add a new > step, and its getId() method would deal with its position. So how does getId() solve anything? In your example they added a set after step_01_doThis(), now if we fix class we add step after step_01_doThis() manually. So would it break the client? Does our step suppose to be before or after his? Having name being API means they can override it. If they cannot it is limited extensibility anyway. Numbers or not, does not matter in this case. I don't specifically going to insist on numbers, if we want to do it manually we can. However I still think having step.id == method name is very elegant solution, saving us lots of coding. > > > e) re-order steps > > override initializeSteps, call super, then user can find a step index using > > findStepIndex method and re-order in array. Override getSteps to return new array > > Seems ok, but I think that will be non-trivial to track the order of steps. > But not a show-stopper. Actually we need more method for that, probably addStep(Step,index) removeStep(index) and setSteps(Step[]) If somebody does not want reflection they can use this instead. > > Please turn on API tooling for patches. > > Why not complete the rm in each step, instead of returning a boolean to > indicate if the rm should be completed after execute() This adds more > complexity to an already complex DSF requirement. So we call method from other step. If it returns true we can continue. If you think it is not good I can rever to rm.done. I wanted universal safety net there - if they throw exception - catch it and do a "done". But I guess it can be done independently. > > About overriding entire Steps instead of execute(), if we look at > step_15_setRemoteTargetConnection(), it only allows to replace the entire step, > instead of overriding it. We could make it overridable by making the class > RemoteTargetConnection protected. I agree all classes should be protected, so people can use them too. I think method in step_01_doThis() class should be public too, or we can even move them out. > I personally prefer to keep the code > relevant to a step inside that step. I think we can override the full steps, > but still use your naming scheme for the step name. But let's leave this for > later. If you don't want to make class public you define a class INSIDE the step method. > > Have you used this patch to create your own overriding of FLS? I think we > should try it before choosing a final solution, to make sure we can override as > we expect. No, I just tested that gdb still works. It is missing some API methods, because if we allow to override it we have to give getters for its fields, because they used in steps, otherwise it is kind of useless.
> We can argue what is or is not "meaningful". But the following is a simple way > to satisfy a)-e) with the existing code. > Say you want to use the default steps with indexes 0, 2 and 5 as the steps 1,2 > and 3 in your sequence and insert a custom step at position 0. You need to do > the following in the class that extends FinalLaunchSequence: > > private Step[] fMySteps; > > @Override > public Step[] getSteps() { > if ( fMySteps == null ) { > Step[] defaultSteps = super.getSteps(); > fMySteps = new Step[4]; > fMySteps[0] = new YourCustomStep(); > fMySteps[1] = defaultSteps[0]; > fMySteps[2] = defaultSteps[2]; > fMySteps[3] = defaultSteps[5]; > } > return fMySteps; > } > > I agree, this code is a hack and not "user friendly". Having a repository of > steps accessible by names or by ids would be enough. > I don't think the idea of having indexes hard-coded into the step names to get > compile time errors when the order of the original class changes is a good one. > Note that you didn't include it into your requirements either. I guess I am missing requirement new step should be done with minimal coding. Current solution does not provide access to any members, which steps are using. So in order to implement a step that does something slightly different we have to copy all steps that do set-up this fields, and copy all private fields. Which leaves not much else.
(In reply to comment #8) > I guess I am missing requirement new step should be done with minimal coding. > Current solution does not provide access to any members, which steps are using. > So in order to implement a step that does something slightly different we have > to copy all steps that do set-up this fields, and copy all private fields. > Which leaves not much else. As I said before this is not a solution. I agree, the current implementation of FLS should provide access to some commonly used members. Implementing each step as a separate class would make it easier to focus on the functionality of each particular step.
(In reply to comment #4) > I don't really have a strong opinion one way or the other on Alena's proposed > solution, but one option to consider is to use java 5 annotation instead of > method names for the step ordering. > > Annotations can be accessed through reflections APIs but they at least have > some compiler checking support. That would be even nicer. However, I'm still hoping we can avoid using reflection. Let's see. (In reply to comment #5) > Also, if you come to a consensus on this let's also consider generalizing the > solution to the Sequence class itself. As this discussion illustrates the > current Sequence class is somewhat awkward to use and extend. OK
(In reply to comment #6) > I agree, this code is a hack and not "user friendly". Having a repository of > steps accessible by names or by ids would be enough. > I don't think the idea of having indexes hard-coded into the step names to get > compile time errors when the order of the original class changes is a good one. I'm also of this opinion.
(In reply to comment #7) > > > > We will probably end-up with conflicts here. Say FLS has step_01_doThis() and > > step_02_doThat(). > > An overriding class would create step_01_01_doSomethingElse(). That all works. > > But then, we decide to add a step to FLS between 1 and 2, we would create > > step_01_01_doThatOtherThing(). > > > > The order of step_01_01_doThatOtherThing() and step_01_01_doSomethingElse() > > become > > 'random', based on what name happened to be chosen after the naming scheme > > pattern. > > > > This would only happen rarely, but still seems not very precise to me. > > > > If I understand correctly, putting ## in the method names allows to keep the > > methods ordered. Also, using reflection allows to add steps from any base or > > overriding class and it will be picked up; I assume that is why you use > > reflection. Instead though, why not use Pawel and Mikhail's suggestion of > > Step.getId(), which could also be sorted alpha-numerically. That way, the name > > would not be an API. As for adding a new step, the base class should have an > > includeStep() method that would be used by overriding classes to add a new > > step, and its getId() method would deal with its position. > > So how does getId() solve anything? In your example they added a > set after step_01_doThis(), now if we fix class we add step after > step_01_doThis() manually. So would it break the client? Does our step suppose > to be before or after his? You're right, my suggestion does not improve this 'problem'. But using getId() would avoid reflection and avoid making the order API, so I think we should go for that. > Having name being API means they can override it. If they cannot it is limited > extensibility anyway. Numbers or not, does not matter in this case. Yes, of course we would need to have a name for each step which would be an API. Let's just keep this name separate from the ordering. So instead of step_01_fetchTracker() we would have step_fetchTracker() and use getId() for the ordering. Which makes me thing that getId() is not right. Maybe we need a method getOrderId() or something. > I don't specifically going to insist on numbers, if we want to do it manually > we can. However I still think having step.id == method name is very elegant > solution, saving us lots of coding. The problem is that when DSF-GDB needs to change the order of an existing step, it should not need to change the name, or else we'll break the API. Say the new FLS has step_01_fetchTracker() and step_02_fetchSession() and we realize we should fetch the session first, how can the new FLS do this? On the other hand if we have step_fetchTracker() and step_fetchSession(), we can order them any way we want, and change what getOrderId() returns. > > > e) re-order steps > > > override initializeSteps, call super, then user can find a step index using > > > findStepIndex method and re-order in array. Override getSteps to return new array > > > > Seems ok, but I think that will be non-trivial to track the order of steps. > > But not a show-stopper. > Actually we need more method for that, probably > addStep(Step,index) > removeStep(index) > and > setSteps(Step[]) Yes, we'll need a good API to make it easy to customize the order. > If somebody does not want reflection they can use this instead. > > > > > > Please turn on API tooling for patches. > > > > Why not complete the rm in each step, instead of returning a boolean to > > indicate if the rm should be completed after execute() This adds more > > complexity to an already complex DSF requirement. > > So we call method from other step. If it returns true we can continue. I see. Although interesting, I think it complicates more than what it is worth. Each step should remain a single executor call and chaining them together would require multiple executor calls. > If you think it is not good I can rever to rm.done. I wanted universal > safety net there - if they throw exception - catch it and do a "done". > But I guess it can be done independently. The 'safety net' does not fully work either, because if the method returns false, but an exception is thrown before rm.done() is called., we won't deal with it properly. Let's just keep it that people have to make sure rm.done() is called properly in their own code. > > About overriding entire Steps instead of execute(), if we look at > > step_15_setRemoteTargetConnection(), it only allows to replace the entire step, > > instead of overriding it. We could make it overridable by making the class > > RemoteTargetConnection protected. > > I agree all classes should be protected, so people can use them too. > I think method in step_01_doThis() class should be public too, or we can even > move them out. > > > I personally prefer to keep the code > > relevant to a step inside that step. I think we can override the full steps, > > but still use your naming scheme for the step name. But let's leave this for > > later. > If you don't want to make class public you define a class INSIDE the step > method. I guess so. But again :-), why not override the entire step? I think we'll need that if we want to use Step.getOrderId() > > Have you used this patch to create your own overriding of FLS? I think we > > should try it before choosing a final solution, to make sure we can override as > > we expect. > No, I just tested that gdb still works. It is missing some API methods, > because if we allow to override it we have to give getters for its fields, > because they used in steps, otherwise it is kind of useless. Once we think we have a good solution, we should try it.
Lets say you won and we not going to have numbers in steps. We down to two options: a) using methods only, call them from step using reflection, b) using method that would create step classes. i.e. a) step_fetchTracker() { ... } ... initializeSteps() { fStep[0] = new NamedStep("step_fetchTracker"); ... } b) Step getStepFetchTracker() { return new Step() { void execute() { ... } } } initializeSteps() { fStep[0] = getStepFetchTracker(); ... } As I understand 2 vs 1 (so far) in favor of approach b). Now lets get to a-e features: b,c) is the same override the method d) would be something like getStepFetchTracker().execute() which is ok a and e) with my approach you can do something like: insertStepAfter(step,"step_fetchTracker"); or int index1 = getStepIndex("step_fetchTracker"); int index2 = getStepIndex("step_fetchSomethingElse"); swapSteps(index1,index2); So with no-reflection approach it is not that easy. So step has to have 2 characteristics: name and position. Position is not known by the step itself, it is defined by array. The only other identification is the name. So every Step class must implement meaningful getId method (not getOrderId!) which would return ident similar to it's step name. We don't need getOrderId because initiazeSteps would take of that. So it will look like this: Step getStepFetchTracker() { return new Step() { void execute() { ... } String getId(){ return "stepFetchTracker"; } } } Now a and b have equal functionality with b) approach having 8 lines of code more per step.
Did you guys see Bug 321259 just get created? It will require to re-order two steps of FLS in DSF-GDB! The coincidence is eery :-)
(In reply to comment #13) [...] > So step has to have 2 characteristics: name and position. > Position is not known by the step itself, it is defined by array. > The only other identification is the name. So every Step class must > implement meaningful getId method (not getOrderId!) which would > return ident similar to it's step name. We don't need getOrderId because > initiazeSteps would take of that. > > So it will look like this: > > Step getStepFetchTracker() { > return new Step() { > void execute() { > ... > } > String getId(){ > return "stepFetchTracker"; > } > } > } I'm not sure it is the best way, but I was thinking we could do String getId() { return "step_01_FetchTracker"; } This would be the same as your original solution, except that the numbering scheme would not be an API. We could easy change String getId() { return "step_01_FetchTracker"; } to become String getId() { return "step_03_FetchTracker"; } > Now a and b have equal functionality with b) approach having 8 lines of code > more per step. You don't agree that b) is better because of API?
(In reply to comment #15) > (In reply to comment #13) > [...] > > So step has to have 2 characteristics: name and position. > > Position is not known by the step itself, it is defined by array. > > The only other identification is the name. So every Step class must > > implement meaningful getId method (not getOrderId!) which would > > return ident similar to it's step name. We don't need getOrderId because > > initiazeSteps would take of that. > > > > So it will look like this: > > > > Step getStepFetchTracker() { > > return new Step() { > > void execute() { > > ... > > } > > String getId(){ > > return "stepFetchTracker"; > > } > > } > > } > > I'm not sure it is the best way, but I was thinking we could do > String getId() { return "step_01_FetchTracker"; } > > This would be the same as your original solution, except that the numbering > scheme would not be an API. We could easy change > String getId() { return "step_01_FetchTracker"; } > to become > String getId() { return "step_03_FetchTracker"; } No we cannot do it. If somebody was relaying on being "after" fetchTraceker, the only way to it is finds its index by name, if we rename a step like - they won't find a step anymore. If we not using numbers in reflection - explicitly specifying sort order in initialize it better and cleaner approach. I.e. fSteps[] = { step_fetchTracker(), ... step_Cleanup() }; This way when we add/remove step we don't care what number is, and we don't care about renaming/change any other steps. So relying on index of step is not API. User always has to do i = getStepIndex("step_fetchTracker") to figure out the step order. > > > Now a and b have equal functionality with b) approach having 8 lines of code > > more per step. > > You don't agree that b) is better because of API? Elaborate? API is almost identical - user can override step methods in both cases. Additionally in case b) if you make anonymous inner class for some more complected cases - users cannot access them, so it is still preferred to have named public inner class outside of step_Method.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #13) > > [...] > > > So step has to have 2 characteristics: name and position. > > > Position is not known by the step itself, it is defined by array. > > > The only other identification is the name. So every Step class must > > > implement meaningful getId method (not getOrderId!) which would > > > return ident similar to it's step name. We don't need getOrderId because > > > initiazeSteps would take of that. > > > > > > So it will look like this: > > > > > > Step getStepFetchTracker() { > > > return new Step() { > > > void execute() { > > > ... > > > } > > > String getId(){ > > > return "stepFetchTracker"; > > > } > > > } > > > } > > > > I'm not sure it is the best way, but I was thinking we could do > > String getId() { return "step_01_FetchTracker"; } > > > > This would be the same as your original solution, except that the numbering > > scheme would not be an API. We could easy change > > String getId() { return "step_01_FetchTracker"; } > > to become > > String getId() { return "step_03_FetchTracker"; } > No we cannot do it. If somebody was relaying on being "after" fetchTraceker, > the only way to it is finds its index by name, > if we rename a step like - they won't find a step anymore. I though that if we went this way, the user would specify a new steps order by choosing an ID with a proper number like step_01_01_myStep. I was basically trying to keep your original suggestion but not making the order API. But I'm not support fond of this solution, so I don't want to give the impression I'm pushing for it :-) > If we not using numbers in reflection - explicitly specifying sort order > in initialize it better and cleaner approach. > I.e. > > fSteps[] = { > step_fetchTracker(), > ... > step_Cleanup() > }; > > This way when we add/remove step we don't care what number is, and we don't > care about renaming/change any other steps. > So relying on index of step is not API. > User always has to do > i = getStepIndex("step_fetchTracker") to figure out the step order. I like this better. However the problem is that when we re-order a step like step_fetchTracker, it will move around user's steps that use getStepIndex("step_fetchTracker"). I find that users adding steps based on other steps is a very brittle approach. Say we now have: step1-"source gdbinit" step2-"set program args" step3-"start program" step4-"set env dir" The extender decides to insert step5-"start recording execution" after step4-"set env dir" They might have chosen that location because they need a) to have the program started b) the env dir set to properly choose the location of the recording file. If we decide to move step4 to be the first step because we need it for gdbinit, the user will end up with (I didn't renumber the step, for clarity): step4-"set env dir" step5-"start recording execution" step1-"source gdbinit" step2-"set program args" step3-"start program" Which will fail because step 5 was automatically moved much too early. I don't see how to make this safer, and still allow new steps and re-ordering to be propagated to extending classes. > > > Now a and b have equal functionality with b) approach having 8 lines of code > > > more per step. > > > > You don't agree that b) is better because of API? > Elaborate? API is almost identical - user can override step methods in both > cases. Additionally in case b) if you make anonymous inner class for some more > complected cases - users cannot access them, so it is still preferred to have > named public inner class outside of step_Method. Having a protected or public method called step_01_fetchTracker, makes the step have to keep the name with "01", which make re-ordering awkward. The ordering should not be API.
> > > You don't agree that b) is better because of API? > > Elaborate? API is almost identical - user can override step methods in both > > cases. Additionally in case b) if you make anonymous inner class for some more > > complected cases - users cannot access them, so it is still preferred to have > > named public inner class outside of step_Method. > > Having a protected or public method called step_01_fetchTracker, makes the step > have to keep the name with "01", which make re-ordering awkward. The ordering > should not be API. We already agreed on "no numbers in the method name". See example for a) there no numbers in the step. There will re-ordered same way - in initializeSteps. As for stepping order I don't think there is any easy solution. Either users can base it on number - not good at all. Or on name. At least if it is symbolic name it some-what what they want. It can be treated as dependency. I can do X only after Y and Z is done (would be a little more code to find out max index for Y and Z). Technically we can create a dependency graph: for each step write pre-requisites, and let smart sorting algorithm figure out step order based on dependencies. This would be fool prove for ordering if users follow same approach, but it is over-engineering.
Created attachment 175597 [details] Patch with reflection support in Sequence. Just to make this discussion even more fun I've prototyped the refelection support based on annotations into the base Sequence class. The annotations are @Execute and @RollBack (listed below). They allow both ordering using a number, which would be used primarily by the base class, and before/after step ordering, to be used by overriding classes. BTW, I don't know if API tooling would consider annotation parameter values as part of the API, but hopefully not. I still agree with all that was said here, but I decided to try this prototype because I find the nested step classes really cumbersome (even more so than the RM ones). It's just a prototype and it's missing support for progress monitors as well as tests for the before/after method of ordering. Also, one big down side of reflection is that sequences using it could not be inlined in a method, though I don't know how much of a problem that is. public static @interface Execute { /** * Order number of the execution of this step. If positive, it will * be used as a key to determine when this execute method should be * run in the sequence. If negative, then <code>beforeStep()</code> or * <code>afterStep()</code> must be specified. * @return */ int value(); /** * ID of this step. It must be specified if other steps rely on this * step to either rollback, or execute before or after this step. * @return */ String id() default ""; /** * Step ID of the step before which this step should be executed. * @return */ String beforeStep() default ""; /** * Step ID of the step after which this step should be executed. * @return */ String afterStep() default ""; } public static @interface RollBack { /** * Step ID of the step which this method rolls back. * @return */ String value(); } and @RollBack
Created attachment 175598 [details] Patch with reflection support in Sequence (with comments in annotation declarations).
(In reply to comment #20) > Created an attachment (id=175598) [details] > Patch with reflection support in Sequence (with comments in annotation > declarations). Can you give an example of step definition that uses this?
Created attachment 175602 [details] Patch with reflection support in Sequence (including tests). (In reply to comment #21) > (In reply to comment #20) > > Created an attachment (id=175598) [details] [details] > > Patch with reflection support in Sequence (with comments in annotation > > declarations). > > Can you give an example of step definition that uses this? Oops, when I updated the patch I left out the test plugin changes. The excerpt is. BTW, the annotations accept constants, so literal strings don't have to be used directly. public class RollBackReflectionSequence extends Sequence { public int fStepCounter; public int fRollBackCounter; public RollBackReflectionSequence() { super(fExecutor); } @Execute(value = 1, id = "step1") public void step1(RequestMonitor rm) { fStepCounter++; rm.done(); } @RollBack("step1") public void rollBack1(RequestMonitor rm) { fRollBackCounter++; rm.done(); } @Execute(value = 2, id = "step2") public void step2(RequestMonitor rm) { fStepCounter++; rm.setStatus(new Status(IStatus.ERROR, DsfTestPlugin.PLUGIN_ID, -1, "", null)); //$NON-NLS-1$ rm.done(); } @RollBack("step2") public void rollBack2(RequestMonitor rm) { fRollBackCounter++; rm.done(); } }
(In reply to comment #22) > Created an attachment (id=175602) [details] > Patch with reflection support in Sequence (including tests). > > (In reply to comment #21) > > (In reply to comment #20) > > > Created an attachment (id=175598) [details] [details] [details] > > > Patch with reflection support in Sequence (with comments in annotation > > > declarations). > > > > Can you give an example of step definition that uses this? > > Oops, when I updated the patch I left out the test plugin changes. The excerpt > is. BTW, the annotations accept constants, so literal strings don't have to be > used directly. > > public class RollBackReflectionSequence extends Sequence { > > public int fStepCounter; > public int fRollBackCounter; > > public RollBackReflectionSequence() { > super(fExecutor); > } > > @Execute(value = 1, id = "step1") > public void step1(RequestMonitor rm) { > fStepCounter++; > rm.done(); > } > > @RollBack("step1") > public void rollBack1(RequestMonitor rm) { > fRollBackCounter++; > rm.done(); > } > > @Execute(value = 2, id = "step2") > public void step2(RequestMonitor rm) { > fStepCounter++; > rm.setStatus(new Status(IStatus.ERROR, DsfTestPlugin.PLUGIN_ID, -1, > "", null)); //$NON-NLS-1$ > rm.done(); > } > > @RollBack("step2") > public void rollBack2(RequestMonitor rm) { > fRollBackCounter++; > rm.done(); > } > } I like the idea to use annotations, however using direct number have big problems which I talked about before a) Users cannot use numbers - because it will conflict with our numbers b) If we add a step we have to go though an renumber all the rest So I still think order them explicitly in FLS is more straight forward - only 20 lines of code (20 steps). Second is id - when you write step1 is method and id=step1 I assume "1" is not significant there and should be just a symbolic name such as stepA (id=stepA) In this case what is the point of repeating the step name? Reflection would know it. The only place it is needed is rollback - because it would be different. So the other two annotation (before & after) is very elegant solution to deal with ordering problem, to place a new step after another they just write a step method and annotation, and to reorder - they override methods and place corresponding annotations on them.
(In reply to comment #23) > I like the idea to use annotations, however using direct number have big > problems which I talked about before I like the idea of numbers for their simplicity, especially if you don't intend your sequence to be overriden. It's the same as placing the numbers in the method names as you did. Yes changing them is more awkward than reordering a list (BTW what is FLS?), but its a tradeoff. For sub-classes, I would discourage use of numbers and rely only on before/after... or override getSteps() completely and imposing a custom order. > a) Users cannot use numbers - because it will conflict with our numbers > b) If we add a step we have to go though an renumber all the rest > So I still think order them explicitly in FLS is more straight forward - only > 20 lines of code (20 steps). > Second is id - when you write step1 is method and id=step1 I assume "1" is not > significant there and should be just a symbolic name such as > stepA (id=stepA) > In this case what is the point of repeating the step name? Reflection would > know it. The only place it is needed is rollback - because it would be > different. I agree the ID is rather redundant. Using method name itself as an ID is also an option, but I wanted to allow the ID to be declared in a string constant (e.g. final static String STEP_ENV = "STEP_ENV")... to reduce the chance of typos. > So the other two annotation (before & after) is very elegant solution to deal > with ordering problem, to place a new step after another they just write > a step method and annotation, and to reorder - they override methods and place > corresponding annotations on them. Using only the before/after to order steps is actually pretty error prone to change (we do this in Wind River's product for in our launch framework). Some more options to consider: - Add a "String[] getStepOrder()" method which would centralize the ordering as a substitute for the step order nubmers. This would be the default ordering, before/after steps could be applied on top of it. - Introduce group-id's in the annotations. Then add a method "Step[] getGroupSteps()" which would be called by the default implementation of getSteps()". Overriding classes could then override getSteps() to customize order without breaking up groups. Sky's the limit on this one :-)
(In reply to comment #19) > Created an attachment (id=175597) [details] > Patch with reflection support in Sequence. > > Just to make this discussion even more fun I've prototyped the refelection > support based on annotations into the base Sequence class. I haven't had time to look at it but it sounds exciting. I'll give it a go early next week. FLS = FinalLaunchSequence :-)
(In reply to comment #25) > FLS = FinalLaunchSequence :-) LOL, you know that something is a menace when it's turned into a TLA.
(In reply to comment #19) > Just to make this discussion even more fun I've prototyped the refelection > support based on annotations into the base Sequence class. That's pretty cool! It would be nice to have it just for the cool effect :-) (In reply to comment #24) > (In reply to comment #23) > > I like the idea to use annotations, however using direct number have big > > problems which I talked about before > > I like the idea of numbers for their simplicity, especially if you don't intend > your sequence to be overriden. It's worth pointing out that this solution is not only about allow to extend an existing sequence, but about making the entire use of Sequence simpler. One could write a sequence containing methods instead of Steps. > It's the same as placing the numbers in the > method names as you did. Yes changing them is more awkward than reordering a > list (BTW what is FLS?), but its a tradeoff. For a large sequence like FLS, it would be annoying to have to go and reorder the steps and change all the numbers. Using beforeStep/afterStep would be better; or choosing our step numbers and leaving room between them, e.g., step1 would be 10, step2 20, etc, then we can insert a step without re-numbering. > > a) Users cannot use numbers - because it will conflict with our numbers Good point. > > b) If we add a step we have to go though an renumber all the rest > > So I still think order them explicitly in FLS is more straight forward - only > > 20 lines of code (20 steps). We've talked about so many things, I'm confused about which explicit ordering you talk about here. > > Second is id - when you write step1 is method and id=step1 I assume "1" is not > > significant there and should be just a symbolic name such as > > stepA (id=stepA) > > In this case what is the point of repeating the step name? Reflection would > > know it. The only place it is needed is rollback - because it would be > > different. > I agree the ID is rather redundant. Using method name itself as an ID is also > an option, but I wanted to allow the ID to be declared in a string constant > (e.g. final static String STEP_ENV = "STEP_ENV")... to reduce the chance of > typos. Although string constants are a good idea, my guess is that most people won't use them so they won't get the compile-time check for typos. I think this solution would benefit from some runtime-checking, to make sure any roolback step is properly matched to an execute step. There are a couple of other runtime-checks that may be good to have: - no duplicate step number - annotation params checks (e.g., if no value, then one and only one of afterStep/beforeStep) - etc > > So the other two annotation (before & after) is very elegant solution to deal > > with ordering problem, to place a new step after another they just write > > a step method and annotation, and to reorder - they override methods and place > > corresponding annotations on them. > Using only the before/after to order steps is actually pretty error prone to > change (we do this in Wind River's product for in our launch framework). Yes, this is still very brittle, as shown in my re-ordering example of comment #17. But again, I personally don't see a clearly better solution. > Some more options to consider: > - Add a "String[] getStepOrder()" method which would centralize the ordering as > a substitute for the step order nubmers. This would be the default ordering, > before/after steps could be applied on top of it. That may prove simpler for a base implementation of Sequence. As for overriding classes, I'm can't tell if it will be any better or the same. We'd have to try it out, if we get to that point. > - Introduce group-id's in the annotations. Then add a method "Step[] > getGroupSteps()" which would be called by the default implementation of > getSteps()". Overriding classes could then override getSteps() to customize > order without breaking up groups. This I like. I guess I should since it is in-line with the subSequence suggestion that I had made for FLS. I think this would most probably improve the current prototype to make it less brittle for overriding classes. > BTW, I don't know > if API tooling would consider annotation parameter values as part of the API, > but hopefully not. We'll have to try that before choosing this solution. > Also, one big down > side of reflection is that sequences using it could not be inlined in a method, > though I don't know how much of a problem that is. Can't we do: Sequence newSeq = new Sequence(getExecutor(), requestMonitor) { @Execute(value = 1, id = "step1") public void step1(RequestMonitor rm) { rm.done(); } }; getExecutor().execute(newSeq); although I'm getting an IllegalAccessException when I try it... I'm not sure why. To conclude, I think what would be nice would be to try this idea with FLS and see if it makes it easier to override. Elena, do you have time to try it out?
Try out what? I made comments about Pawel patch already, I did not need to try it. Btw instead of integer numerical id we can use hierarchical of string type, i.e 1 1.1 1.2 2 etc, that would solve problem with re-numbering and user steps order. And it does not have to be unique - if user defines same step number - it will be executed in random order (between ones that have same number).
(In reply to comment #28) > Try out what? I made comments about Pawel patch already, I did not need to try > it. Well, FLS would need to be converted to this new scheme before it would benefit from it. And by doing that we may figure out it does not solve our extensibility problem. That being said, using annotation in Sequence does not require that it make FLS easier to extend, so if we find value in that solution in itself, I say go for it. > Btw instead of integer numerical id we can use hierarchical of string type, i.e > 1 > 1.1 > 1.2 > 2 > etc, that would solve problem with re-numbering and user steps order. And it > does not have to be unique - if user defines same step number - it will be > executed > in random order (between ones that have same number). Would we need to use float for this? How would we compare strings like 2, 2.1 and 10 so that they are ordered numerically (2, 2.1, 10) and not lexically (10, 2, 2.1).
(In reply to comment #29) > > Well, FLS would need to be converted to this new scheme before it would benefit > from it. And by doing that we may figure out it does not solve our > extensibility problem. Well it is easy. I can modify my patch - to use annotations instead of method names and fix some places where I replaces requestMonitor.done with return. > > Would we need to use float for this? No. Float cannot handle 1.1.1 > How would we compare strings like 2, 2.1 and 10 so that they are ordered > numerically (2, 2.1, 10) and not lexically (10, 2, 2.1). We write a comparator.
Cool. So from what I gother in this conversation, we have consensus on the following changes in the Sequence patch: - remove the id parameter and use the the function name instead - add runtime checks for bindings of before/after, id# etc. - ... anything else? On the step number question. The reason I made it an integer is because I thought we agreed that we didn't want the step numbers to become an API. This effectively means that Sequence extensions would not use the numbers for ordering the steps in overriding classes. But after following this discussion, I don't see how that would be enforceable since using numbers is so logical. So how about this idea: - Get rid of the order number, - Introduce the idea of hierarchical groups as an additional annotation with a String ID and an optional parent group - Add a "String[] getOrder(String group)" method which would return the order of steps in a group, or the order of sub-groups within a group. All overriding of order behavior would be localized to this one method, but it would avoid a dependency on the full ordering of all the steps.
(In reply to comment #31) > Cool. > So from what I gother in this conversation, we have consensus on the following > changes in the Sequence patch: > - remove the id parameter and use the the function name instead > - add runtime checks for bindings of before/after, id# etc. > - ... anything else? I think that is it. > On the step number question. The reason I made it an integer is because I > thought we agreed that we didn't want the step numbers to become an API. This > effectively means that Sequence extensions would not use the numbers for > ordering the steps in overriding classes. But after following this discussion, > I don't see how that would be enforceable since using numbers is so logical. > > So how about this idea: > - Get rid of the order number, > - Introduce the idea of hierarchical groups as an additional annotation with a > String ID and an optional parent group That sounds good in theory, but it may prove complex to find proper groups. I guess we can see from FLS how useful it proves. I don't think it will be useful for other sequences since they are usually only a couple of steps. > - Add a "String[] getOrder(String group)" method which would return the order of > steps in a group, or the order of sub-groups within a group. All overriding of > order behavior would be localized to this one method, but it would avoid a > dependency on the full ordering of all the steps. Ok Thanks!
Created attachment 175875 [details] Patch with anootation based sequence including FLS. This patch follows on the suggestion to ditch the order numbers and before/after stuff and use getGroupOrder(). I ported FLS to use this also. Personally I like the getGroupOrder() much better than other methods of ordering, because it centralizes it. Still without compiler support its still going to be error prone. As far as groups, I'm not sure yet if I like it. It seems to me that if you need groups you should really break up your sequence into multiple classes. FLS is not terribly long (700 loc), but it seems long enough that breaking it up would make it much easier to understand what's going on.
(In reply to comment #33) > Created an attachment (id=175875) > Patch with anootation based sequence including FLS. Even cooler! > This patch follows on the suggestion to ditch the order numbers and before/after > stuff and use getGroupOrder(). > > I ported FLS to use this also. Thanks for doing that. Your workspace wasn't quite up-to-date though so the patch does not apply cleanly. The problem is simply that the setEnv step was moved before the gdbinit step, but it's hard for me to merge the changes like that. If you attach the entire FLS file of yours that will be fine. > Personally I like the getGroupOrder() much > better than other methods of ordering, because it centralizes it. At first glance, I'm very impressed that FLS seems easier to read now! The getGroupOrder() method gives an overview that we didn't have before. And makes re-ordering even easier. > Still without > compiler support its still going to be error prone. As far as groups, I'm not > sure yet if I like it. It seems to me that if you need groups you should really > break up your sequence into multiple classes. FLS is not terribly long (700 > loc), but it seems long enough that breaking it up would make it much easier to > understand what's going on. You mean to have multiple classes be run within a single sequence? Kind of like GDBControl? I'll do a more detailed review tomorrow.
I was wondering if we really need the optional group value() in @Execute? What is the value of specifying a group for a method? Looking at getGroupOrder() of FLS, the steps are mapped to a group in there already.
I like this, this is pretty much what I proposing but with cool annotations. I am not sold on "group" idea. I am not sure how it is better then just plain list. Especially with only one group so far. To complete the picture we also need re-ordering API and getters for FLS fields.
(In reply to comment #34) > You mean to have multiple classes be run within a single sequence? Kind of like > GDBControl? Not quite, GDBContorl still has only one Sequence. I was thinking that we could create something like InitSequence, then have a step that executes this sequence. The problem with creating sub-sequences is that currently all steps access the same the class data, so creating sub-sequences would only be meaningful if a more formal API is created for the shared data. I don't really know if this is something you'd want to do. It would probably make sense only if FLS were to grow significantly. (In reply to comment #35) > I was wondering if we really need the optional group value() in @Execute? What > is the value of specifying a group for a method? Looking at getGroupOrder() of > FLS, the steps are mapped to a group in there already. Good point! Logically it is redundant.] (In reply to comment #36) > I like this, this is pretty much what I proposing but with cool annotations. I agree, I give you the full credit for flattening the sequence :-) > I am not sold on "group" idea. I am not sure how it is better then just plain > list. Especially with only one group so far. I don't know FLS intimately, so I only created one group as an example. You guys would know better how to break it up into logical chunks. > To complete the picture we also need re-ordering API and getters for FLS > fields. I was hoping that we wouldn't need any additional numbers/afters/befores besides what's there already. Sub classes could just override getGroupOrder() then add/remove steps to the list provided by super-class. Having the step names as Strings should make this mechanism pretty easy yet very powerful.
> I don't know FLS intimately, so I only created one group as an example. You > guys would know better how to break it up into logical chunks. I still don't understand why having group is better. It does not simply anything. > > > To complete the picture we also need re-ordering API and getters for FLS > > fields. > I was hoping that we wouldn't need any additional numbers/afters/befores > besides what's there already. Sub classes could just override getGroupOrder() > then add/remove steps to the list provided by super-class. Having the step > names as Strings should make this mechanism pretty easy yet very powerful. So lets say I want to insert a step in main sequence code would look very ugly getGroupOrder(id){ String steps[] = super.getGroupOrder(); String newsteps = new String[steps.length+1]; // insert step after bla int j=0; for(i=0;i<steps.length;i++,j++) { newsteps[j]=step[j]; if (steps[i].equals("stepBla")) { newstep[++j]="stepMy"; } } return newsteps; } If we have some API we could have done getGroupOrder(id){ String steps[] = super.getGroupOrder(id); return insterStepAfter(steps, "stepMy","stepBla"); }
(In reply to comment #38) > > I don't know FLS intimately, so I only created one group as an example. You > > guys would know better how to break it up into logical chunks. > I still don't understand why having group is better. It does not simply > anything. I agree, it's not so much about simplification as much as extensibility. If your base class breaks things up into groups, then in the sub-class you can say: "insert my step after group A". Then if the base class changes what steps are executed in group A, your sub-class will still work as intended. I.e. rather than relying on specific steps to insert your steps into the sequence, you can rely on a higher-level logic. > So lets say I want to insert a step in main sequence code would look very ugly Now I see what you mean. Yes, I think that makes perfect sense.
> > So lets say I want to insert a step in main sequence code would look very ugly > Now I see what you mean. Yes, I think that makes perfect sense. Actually if we make getGroupOrder return List<String> instead of String[] that would solve most of the problems List<String> getGroupOrger() { List<String> res = super.getGroupOrger(); res.add( res.indexOf("stepBla") + 1,"stepMy"); res.remove("stepX"); }
Created attachment 175967 [details] Full FinalLaunchSequence file. (In reply to comment #40) > > > So lets say I want to insert a step in main sequence code would look very ugly > > Now I see what you mean. Yes, I think that makes perfect sense. > > Actually if we make getGroupOrder return List<String> instead of String[] > that would solve most of the problems It would solve the problem of overriding, but it would make the step order declarations uglier... though I don't really care much either way. Unfortunately, I'll be going away on vacation starting tomorrow and coming back Aug 30th. So unless you want to wait that long, I'll leave it to you two to work out the remaining details.
(In reply to comment #37) > (In reply to comment #34) > > You mean to have multiple classes be run within a single sequence? Kind of like > > GDBControl? > > Not quite, GDBContorl still has only one Sequence. I was thinking that we > could create something like InitSequence, then have a step that executes this > sequence. That was the piece I was missing. How to chain sub-sequences into a single executable call. > The problem with creating sub-sequences is that currently all steps > access the same the class data, so creating sub-sequences would only be > meaningful if a more formal API is created for the shared data. I don't really > know if this is something you'd want to do. It would probably make sense only > if FLS were to grow significantly. I don't expect FLS to change much. I think we can start by keeping it as a single class, using the ReflectionSequence, as see how things go. > (In reply to comment #35) > > I was wondering if we really need the optional group value() in @Execute? What > > is the value of specifying a group for a method? Looking at getGroupOrder() of > > FLS, the steps are mapped to a group in there already. > Good point! Logically it is redundant. Do you prefer to keep it or remove it? I'd remove it for simplicity, but that's me. > Created an attachment (id=175967) [details] > Full FinalLaunchSequence file. Thanks. > Unfortunately, I'll be going away on vacation starting tomorrow and coming back > Aug 30th. So unless you want to wait that long, I'll leave it to you two to > work out the remaining details. Are you comfy with the changes as they are now? If so, Elena and I can workout the final details and commit it eventually. Have a good vacation!
Created attachment 175975 [details] Pawel's patch updated to HEAD I updated Pawel's patch for HEAD. I also changed all the new FLS methods from public to protected. I think we only want to access those when overriding the class. I also made all fields of FLS private instead of having some of them package-private, which was an omission instead of an intent. No other changes.
(In reply to comment #42) > > Good point! Logically it is redundant. > > Do you prefer to keep it or remove it? I'd remove it for simplicity, but > that's me. I agree, remove it. > Are you comfy with the changes as they are now? If so, Elena and I can workout > the final details and commit it eventually. Yes, you're the main customers here, so it's really up to you. (In reply to comment #43) > I also changed all the new FLS methods from public to protected. I'm not sure if that'll work. It may throw IllegalAccessException... that's the price of reflection. In the past I've done stuff like change the Method object to indicate that its public instead of private, so that I could call it, but it seems a little too much like a hack.
> (In reply to comment #43) > > I also changed all the new FLS methods from public to protected. > I'm not sure if that'll work. It may throw IllegalAccessException... that's > the price of reflection. In the past I've done stuff like change the Method > object to indicate that its public instead of private, so that I could call it, > but it seems a little too much like a hack. Of course, you are right: java.lang.IllegalAccessException: Class org.eclipse.cdt.dsf.concurrent.ReflectionSequence$ReflectionStep can not access a member of class org.eclipse.cdt.dsf.gdb.launching.FinalLaunchSequence with modifiers "protected" I'll set them back to public.
Created attachment 175976 [details] Pawel's patch updated to HEAD using plublic Updated patch to put back 'public' for methods with annotations.
(In reply to comment #45) > > (In reply to comment #43) > > > I also changed all the new FLS methods from public to protected. > > I'm not sure if that'll work. It may throw IllegalAccessException... that's > > the price of reflection. In the past I've done stuff like change the Method > > object to indicate that its public instead of private, so that I could call it, > > but it seems a little too much like a hack. > > Of course, you are right: > > java.lang.IllegalAccessException: Class > org.eclipse.cdt.dsf.concurrent.ReflectionSequence$ReflectionStep can not access > a member of class org.eclipse.cdt.dsf.gdb.launching.FinalLaunchSequence with > modifiers "protected" > > I'll set them back to public. Don't worry about reflection, we just have to change reflection method which is used, see my class in prev. patch. It can even use private methods.
(In reply to comment #47) > Don't worry about reflection, we just have to change reflection method which is > used, see my class in prev. patch. It can even use private methods. Do you mean: + boolean callMethod(Method method, RequestMonitor rm) { + try { + Object result = method.invoke(obj, rm); + ... Could you elaborate, I don't get the difference.
No getMethods() returns only public methods, you need to use getDeclaredMethods (code has to be modified a bit)
(In reply to comment #49) > No getMethods() returns only public methods, you need to use getDeclaredMethods > (code has to be modified a bit) It is with getDeclaredMethods() that I got: > java.lang.IllegalAccessException: Class > org.eclipse.cdt.dsf.concurrent.ReflectionSequence$ReflectionStep can not access > a member of class org.eclipse.cdt.dsf.gdb.launching.FinalLaunchSequence with > modifiers "protected" Do you mean that we change the visibility of the method before we call it? As Pawel mentioned in comment #44: > In the past I've done stuff like change the Method > object to indicate that its public instead of private, so that I could call it, > but it seems a little too much like a hack.
It worked before (my 1st patch uses protected methods). I have to look again. Maybe it depends on which class calls invoke. + TreeMap<String, Method> hash = new TreeMap<String, Method>(); Method>(); + Class clazz = getClass(); + while (!clazz.equals(MethodBasedSequence.class)) { + Method[] methods = clazz.getDeclaredMethods(); + for (int i = 0; i < methods.length; i++) { + Method method = methods[i]; + String name = method.getName(); + if (name.startsWith(STEP_PREFIX)) { + if (!hash.containsKey(name)) + hash.put(name, method); + } + } + clazz = clazz.getSuperclass(); + } Code above would all method starting with STEP_PREFIX. For overridden methods (even private) user class has precedence.
hmm it looks very similar. In my patch Step extending class that calls invoke is not private, don't know if it matters.
Created attachment 175980 [details] Updated patch removing value() from @Execute I removed value() from @Execute. It actually simplified the ReflectionSequence class in a non-negligeable way and I believe it took care of the TODO by using HashMaps instead of List. I also fixed some javadoc to add more details and to remove the one from ReflectionStep#getSteps() since it is the same as the one in super.getSteps(). And I fixed ReflectionStep#rollBack which had the wrong text in the error status of the rm. The other thing I think we should do is that ReflectionStep#rollBack should check for fRollbackMethod == null and do rm.done() or super.rollBack() in that case. I have to go home now, so I won't have time today.
(In reply to comment #53) > Created an attachment (id=175980) [details] > Updated patch removing value() from @Execute Nice, can you also preserve the changes to the DsfSequenceTests. I would like to extend them when I get back to test the full ReflectionSequence.
I figured out why reflection worked for me - because my class was in the same package as FLS. If you move ReflecionSequence in the same package - it would work too. There is a bit hacky way how to make it work - you need to define function "callMethod" @Override public void callMethod(Method method, RequestMonitor rm) throws IllegalAccessException, InvocationTargetException { method.invoke(FinalLaunchSequence.this, rm); } in the FLS itself (and ReflecionSequence ) and call it instead of calling invoke directly from the Step. However it would be same problem for the client - so it is not worth fighting - just leave them public. So I am good with a patch but we still need getter for all the fields in FLS and step order API (or return List) from getGroupOrder. Btw I don't like the name getGroupOrder. Should be probably getStepsOrder(String group).
Created attachment 176051 [details] Patch removing value() from @Execute and including tests > Nice, can you also preserve the changes to the DsfSequenceTests. I would like > to extend them when I get back to test the full ReflectionSequence. Sorry, my mistake.
Created attachment 176052 [details] mylyn/context/zip
(In reply to comment #55) > I figured out why reflection worked for me - because my class was in the same > package as FLS. If you move ReflecionSequence in the same package - it would > work too. There is a bit hacky way how to make it work - you need to define > function "callMethod" > > @Override > public void callMethod(Method method, RequestMonitor rm) > throws IllegalAccessException, InvocationTargetException { > method.invoke(FinalLaunchSequence.this, rm); > } > in the FLS itself (and ReflecionSequence ) and call it instead of calling > invoke directly from the Step. However it would be same problem for the client > - so it is not worth fighting - just leave them public. Agreed. > So I am good with a patch but we still need getter for all the fields in FLS > and step order API (or return List) from getGroupOrder. I won't be able to work on this until late next week, so if you'd like to take over Elena, feel free. > Btw I don't like the name getGroupOrder. Should be probably > getStepsOrder(String group). Me too, I didn't like that name too much. getOrder() or getStepsOrder() woudl be good for me.
May I ask that before this patch is checked in, some additional comments be added. In particular, comments on ReflectionSequence class, as well as Execute and Rollback interfaces inside it, as extremely brief and do not actually explain how this class should be used.
Created attachment 176399 [details] More updates This patch checks for fRollbackMethod == null in ReflectionStep and adds a test for this case. I renames getGroupOrder() to getExecutionOrder(). It adds javadoc, including an example at the top to show how to use the file. No changes to FLS yet and no step order API. Volodya, does the javadoc make things clearer?
Marc, the extra comments do clarify things, thanks!
Marc can you apply the patch? We can add getters later.
(In reply to comment #62) > Marc can you apply the patch? We can add getters later. I hadn't looked much at the new FLS for this patch, so I started doing so now. I thought I would take this opportunity to merge the stepGetGDBBackend(), stepGetCommandControl() and stepGetProcessService() into a single step, which would make more sense. But when doing that, I forgot to update my getExecutionOrder(), which gave a stack overflow eventually. The reason is that if a 'step' returned by getExecutionOrder() does not exist in the sequence, the new ReflectionSequence assumes it is a group and calls getExecutionOrder() with that group name. Which lead to a infinite loop, which required me to use the debugger to figure out. Maybe we need to have proper support for groups in ReflectionSequence? We could have another annotation. That way, a step that is being called by not tagged as a group could be immediately flagged as a missing step. What do you think?
Personally I think having "groups" is overkill there. I would just do a plain sequence and see how it goes from here.
(In reply to comment #64) > Personally I think having "groups" is overkill there. I would just do a plain > sequence and see how it goes from here. That won't help. A typo in the getExecutionOrder() will cause a stack overflow. But I think we can make that an short-term improvement, not to delay this patch. I will commit soon.
Created attachment 177763 [details] Latest update of patch with FLS cleanup of names Here's the latest patch with a cleanup of FLS to use names I found more descriptive, and to use javadoc.
Created attachment 177770 [details] Tiny update for a name in FLS I changed stepSetEnvironment() for stepSetEnvironmentVariables()
Created attachment 177779 [details] Update of GDBJtagDSFFinalLaunchSequence This patch updates GDBJtagDSFFinalLaunchSequence to extend FLS instead of copying it. It illustrates some of the problems with our solution: 1- as Elena mentioned, we are missing an API to insert/remove steps 2- we have to be careful about name clashes. For example, the FLS.stepGetServices() is essential for success, but the name can mistakenly be re-used by the extending class, to fetch other services. This will break things. We may want to make certain steps of FLS final to prevent overriding. I'm thinking of the two initialization steps. 3- overriding getExecutionOrder() may be necessary depending on point 1, and if so, may need to be improved in FLS. 4- getters are not really needed in my eyes for FLS, as this patch shows.
I am not sure what you mean by as this patch shows. Original idea is to use this technique to override on the steps. So if I override a step that uses one of the services - they all private fields, how do I suppose to access them? I don't like the ide to override and COPY all command control setup methods.
(In reply to comment #69) > I am not sure what you mean by as this patch shows. Original idea is to use > this technique to override on the steps. So if I override a step that uses one > of the services - they all private fields, how do I suppose to access them? I > don't like the ide to override and COPY all command control setup methods. Services can easily be obtained by using the DsfServicesTracker. Accessing the data from FLS only forces FLS to keep that field available. In your own sequence you may need to access other services that are not used in FLS, and you would have to fetch them yourself anyway. I agree that when overriding a step, the first instinct is to re-use the same variables, but I don't think fetching the service yourself is a big deal, and it leaves more freedom for FLS to change. You say "COPY all command control setup methods", but isn't there only three in FLS? Doesn't sound too bad (especially since you probably won't need all three).
OK. Lets just commit the whole thing let people complain (or not) if we miss something. To solve missing step/group just add prefix to a group (I think I was suggesting it somewhere in the thread). I.e. group should be called ":INIT" (i.e. start with ":" or some symbol which cannot be part of the ident)
(In reply to comment #71) > OK. Lets just commit the whole thing let people complain (or not) if we miss > something. To solve missing step/group just add prefix to a group (I think I > was suggesting it somewhere in the thread). I.e. group should be called > > ":INIT" (i.e. start with ":" or some symbol which cannot be part of the ident) Still won't work. If the user has a typo in a step, we'll look for that group. But it's ok, I've fixed it by re-introducing Pawel's GROUP-DEFAULT (which I renamed GROUP_TOP_LEVEL), so the FLS now looks for proper group names and returns null for invalid one, which will cause an exception. I'll post the patch.
Created attachment 177850 [details] More updating This patch addresses the issue of invalid group names, by creating a GROUP_TOP_LEVEL in ReflectionSequence. Also about: > 2- we have to be careful about name clashes. For example, the > FLS.stepGetServices() is essential for success, but the name can mistakenly be > re-used by the extending class, to fetch other services. This will break > things. We may want to make certain steps of FLS final to prevent overriding. > I'm thinking of the two initialization steps. To be more flexible, I chose not to make those steps 'final'. Instead I merged them into a single stepInitializeFinalLaunchSequence() which is named so that people don't override it by mistake. I also removed the GROUP_INIT from FLS. I'll commit this patch to HEAD.
Pawel, can you review the patch I just committed ("More updating")
*** cdt cvs genie on behalf of mkhouzam *** Bug 321084: Create a new ReflectionSequence and have FinalLaunchSequence use it. The goal to to make FinalLaunchSequence easier to extend. [+] ReflectionSequence.java http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/ReflectionSequence.java?root=Tools_Project&revision=1.1&view=markup [*] FinalLaunchSequence.java 1.15 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf-gdb/org.eclipse.cdt.dsf.gdb/src/org/eclipse/cdt/dsf/gdb/launching/FinalLaunchSequence.java?root=Tools_Project&r1=1.14&r2=1.15 [*] DsfSequenceTests.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.tests.dsf/src/org/eclipse/cdt/tests/dsf/concurrent/DsfSequenceTests.java?root=Tools_Project&r1=1.2&r2=1.3
If user made a typo in a step we won't look for this group because it does not stat with ":". It has to be mandatory for group to start with ":". If it does not it is a step and we should give an error that step is missing.
I've opened Bug 324101 to have GDBJtagDSFFinalLaunchSequence extend FinalLaunchSequence which will be a good way to see how our solution actually applies. I'll resolve this bug and we can continue the discussion in Bug 324101.
(In reply to comment #76) > If user made a typo in a step we won't look for this group because it does not > stat with ":". It has to be mandatory for group to start with ":". If it does > not it is a step and we should give an error that step is missing. Sorry, I hadn't understood. That is a good idea. With the GROUP_TOP_LEVEL, I'm not sure it is necessary anymore. We can see how things go.
*** cdt cvs genie on behalf of mkhouzam *** Bug 321084: Updated the example. [*] ReflectionSequence.java 1.2 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/ReflectionSequence.java?root=Tools_Project&r1=1.1&r2=1.2 [*] ReflectionSequence.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/dsf/org.eclipse.cdt.dsf/src/org/eclipse/cdt/dsf/concurrent/ReflectionSequence.java?root=Tools_Project&r1=1.2&r2=1.3
