Again, this looks like it will work, but it requires lot of scrolling and reading to understand how it operates. Better documentation would be helpful, have you considered writing docs/dispatch.txt to document the design decisions? Also, see the comments in the code.
--
Martin SivÃk
msivak@xxxxxxxxxx
Red Hat Czech
Anaconda team / Brno, CZ
----- Original Message -----
> Imagine this scenario, steps A, B and C are scheduled to be executed
> in
> that order. Based on a user's choice A decides to skip_step(B) and
> then
> the user presses "back" during C. This will take us back to A, but
> previously the state of step B would stay 'skipped'. Instead, it is
> desirable upon going back to have all the steps in the same scheduling
> state as they were when the original step was first entered.
>
> This patch addresses that by having each step store a dictionary of
> scheduling changes that happened during that step (a mapping from a
> step
> name to the tuple (from_state, to_state). It is therefore always
> possible
> to reconstruct the original state.
>
> Unittests included.
> ---
> pyanaconda/dispatch.py | 74 ++++++++++++++++++++++++--------
> tests/pyanaconda_test/dispatch_test.py | 41 +++++++++++++++++
> 2 files changed, 97 insertions(+), 18 deletions(-)
>
> diff --git a/pyanaconda/dispatch.py b/pyanaconda/dispatch.py
> index ed943d5..cc0eeb9 100644
> --- a/pyanaconda/dispatch.py
> +++ b/pyanaconda/dispatch.py
> @@ -79,12 +79,14 @@ class Step(object):
> user interface object is told to handle the step (i.e. indirect
> step).
> """
> + self.changes = {} # tracks changes (FROM, TO) in scheduling of other
> steps
> self.name = name
> self.target = target # None for dynamic target (e.g. gui view)
> self._sched = self.SCHED_UNSCHEDULED
>
> def _reschedule(self, to_sched):
> - new_sched = self.sched_state_machine[self._sched][to_sched]
> + s_from = self.sched
> + new_sched = self.sched_state_machine[self.sched][to_sched]
> if new_sched is None:
> raise errors.DispatchError(
> "Can not reschedule step '%s' from '%s' to '%s'" %
> @@ -92,16 +94,17 @@ class Step(object):
> self.namesched(self._sched),
> self.namesched(to_sched)))
> self._sched = new_sched
> + return (self.name, s_from, self.sched)
> @property
> def direct(self):
> return self.target is not None
>
> def done(self):
> - self._reschedule(self.SCHED_DONE)
> + return self._reschedule(self.SCHED_DONE)
>
> def request(self):
> - self._reschedule(self.SCHED_REQUESTED)
> + return self._reschedule(self.SCHED_REQUESTED)
>
> def namesched(self, sched):
> return {
> @@ -112,16 +115,24 @@ class Step(object):
> self.SCHED_DONE : "done"
> }[sched]
>
> + def revert_sched(self, s_from, s_to):
> + assert(self.sched == s_to)
> + self._sched = s_from
> +
> @property
> def sched(self):
> return self._sched
You should state somewhere around schedule/skip/, that they are not to be used outside of dispatch to ensure the history tracking works.
> def schedule(self):
> - self._reschedule(self.SCHED_SCHEDULED)
> + return self._reschedule(self.SCHED_SCHEDULED)
>
> def skip(self):
> - self._reschedule(self.SCHED_SKIPPED)
> + return self._reschedule(self.SCHED_SKIPPED)
This method does no tracking, it only records information passed from somewhere outside. So it should be documented what values are to be passed in, their effect. The name could be changed too probably, either to track_scheduling to match the method from dispatch or maybe to for example record_history?
> + def track_sched(self, step, s_from, s_to):
> + if step in self.changes:
> + s_from = self.changes[step][0]
> + self.changes[step] = (s_from, s_to)
>
> class Dispatcher(object):
>
> @@ -129,6 +140,7 @@ class Dispatcher(object):
> self.anaconda = anaconda
> self.anaconda.dir = DISPATCH_FORWARD
> self.step = None # name of the current step
> + self.stop = False
> # step dictionary mapping step names to step objects
> self.steps = indexed_dict.IndexedDict()
> # Note that not only a subset of the steps is executed for a
> particular
> @@ -187,12 +199,38 @@ class Dispatcher(object):
> self.steps[name] = Step(name, target)
>
> def _advance_step(self):
> - i = self._step_index()
> - self.step = self.steps[i + 1].name
> + if self.step is None:
> + # initialization
> + log.info("dispatch: resetting to the first step.")
> + self.step = self.steps[0].name
> + elif self._step_index() < len(self.steps) - 1:
> + i = self._step_index()
> + if self.dir == DISPATCH_BACK:
> + # revert whatever changed in the current step
> + self._revert_scheduling(self.step)
> + self.step = self.steps[i + self.dir].name
> + if self.dir == DISPATCH_BACK:
> + # revert whatever changed in the step we moved back to
> + self._revert_scheduling(self.step)
> + else:
> + # advancing from the last step
> + self.step = "_invalid_"
> + self.stop = True
> +
> + def _revert_scheduling(self, reverted_step):
> + for (step, (s_from, s_to)) in
You access most of the Step internals using properties and methosd.. why the direct access to changes here?
> self.steps[reverted_step].changes.items():
> + self.steps[step].revert_sched(s_from, s_to)
> + self.steps[reverted_step].changes = {}
>
> def _step_index(self):
> return self.steps.index(self.step)
>
> + def _track_scheduling(self, changes):
> + if self.step is not None:
> + # store the changes into the current step
> + step = self.steps[self.step]
> + map(lambda c: step.track_sched(*c), changes)
> +
> @property
> def dir(self):
> return self.anaconda.dir
> @@ -205,7 +243,8 @@ class Dispatcher(object):
> self.anaconda.dir = dir
>
> def done_steps(self, *steps):
> - map(lambda s: self.steps[s].done(), steps)
> + changes = map(lambda s: self.steps[s].done(), steps)
> + self._track_scheduling(changes)
>
> def go_back(self):
> """
> @@ -231,14 +270,16 @@ class Dispatcher(object):
> return False
>
> def request_steps(self, *steps):
> - map(lambda s: self.steps[s].request(), steps)
> + changes = map(lambda s: self.steps[s].request(), steps)
> + self._track_scheduling(changes)
>
> def run(self):
> self.anaconda.intf.run(self.anaconda)
> log.info("dispatch: finished.")
>
> def schedule_steps(self, *steps):
> - map(lambda s: self.steps[s].schedule(), steps)
> + changes = map(lambda s: self.steps[s].schedule(), steps)
> + self._track_scheduling(changes)
>
> def step_disabled(self, step):
> """ True if step is not yet scheduled to be run or will never be run
> @@ -253,24 +294,21 @@ class Dispatcher(object):
> Step.SCHED_DONE]
>
> def skip_steps(self, *steps):
> - map(lambda s: self.steps[s].skip(), steps)
> + changes = map(lambda s: self.steps[s].skip(), steps)
> + self._track_scheduling(changes)
>
> def step_is_direct(self, step):
> return self.steps[step].direct
>
> def dispatch(self):
> - total_steps = len(self.steps)
> - if self.step == None:
> - log.info("dispatch: resetting to the first step.")
> - self.step = self.steps[0].name
> - else:
> + if self.step:
> log.info("dispatch: leaving (%d) step %s" %
> (self.dir, self.step))
> self.done_steps(self.step)
> - self._advance_step()
> + self._advance_step()
>
> while True:
> - if self._step_index() >= total_steps:
> + if self.stop:
> # installation has proceeded beyond the last step: finished
> self.anaconda.intf.shutdown()
> return
> diff --git a/tests/pyanaconda_test/dispatch_test.py
> b/tests/pyanaconda_test/dispatch_test.py
> index afb2e88..5aea466 100644
> --- a/tests/pyanaconda_test/dispatch_test.py
> +++ b/tests/pyanaconda_test/dispatch_test.py
> @@ -79,6 +79,24 @@ class StepTest(mock.TestCase):
> self.assertRaises(DispatchError, s.done)
> self.assertRaises(DispatchError, s.request)
>
> + def track_sched_test(self):
> + from pyanaconda.dispatch import Step
> + s = Step("first_step")
> + self.assertEqual(s.sched, Step.SCHED_UNSCHEDULED)
> + s2 = Step("second_step")
> + s2.track_sched(*s.skip())
> + self.assertEqual(s.sched, Step.SCHED_SKIPPED)
> + self.assertDictEqual(
> + s2.changes,
> + {'first_step' : (Step.SCHED_UNSCHEDULED, Step.SCHED_SKIPPED)})
> +
> + def revert_sched_test(self):
> + from pyanaconda.dispatch import Step
> + s = Step("first_step")
> + s.request()
> + s.revert_sched(Step.SCHED_UNSCHEDULED, Step.SCHED_REQUESTED)
> + self.assertEqual(s.sched, Step.SCHED_UNSCHEDULED)
> +
> class DispatchTest(mock.TestCase):
> def setUp(self):
> self.setupModules(
> @@ -131,3 +149,26 @@ class DispatchTest(mock.TestCase):
> self.assertFalse(d.step_enabled("betanag"))
> self.assertFalse(d.step_enabled("complete"))
> self.assertTrue(d.step_enabled("filtertype"))
> +
> + def track_scheduling_test(self):
> + from pyanaconda.dispatch import Step
> + d = self._getDispatcher()
> + d.schedule_steps("betanag", "filtertype", "filter")
> + d.step = "betanag"
> + # tested through the request_steps
> + d.request_steps("filtertype", "filter")
> + self.assertDictEqual(
> + d.steps[d.step].changes,
> + {"filtertype" : (Step.SCHED_SCHEDULED, Step.SCHED_REQUESTED),
> + "filter" : (Step.SCHED_SCHEDULED, Step.SCHED_REQUESTED)})
> +
> + def revert_scheduling_test(self):
> + from pyanaconda.dispatch import Step
> + d = self._getDispatcher()
> + d.schedule_steps("betanag", "filtertype", "filter")
> + d.step = "betanag"
> + d.request_steps("filtertype", "filter")
> + d._revert_scheduling("betanag")
> + self.assertEqual(d.steps["filtertype"].sched, Step.SCHED_SCHEDULED)
> + self.assertEqual(d.steps["filter"].sched, Step.SCHED_SCHEDULED)
> + self.assertDictEqual(d.steps[d.step].changes, {})
> --
> 1.7.3.3
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
--
Martin SivÃk
msivak@xxxxxxxxxx
Red Hat Czech
Anaconda team / Brno, CZ
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
