Re: [anaconda][rhel7-branch][PATCH] Add support for autostep and --autoscreenshot (#1234896)

[Jiří Konečný]

On Mon, 2015-07-27 at 18:55 +0200, Martin Kolman wrote:
> diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
> index c827431..9136406 100644
> --- a/pyanaconda/iutil.py
> +++ b/pyanaconda/iutil.py
> @@ -27,6 +27,7 @@ import os.path
>  import errno
>  import subprocess
>  import unicodedata
> +import shutil
>  import string
>  import tempfile
>  import types
> @@ -36,6 +37,7 @@ import signal
>
> from pyanaconda.flags import flags
>  from pyanaconda.constants import DRACUT_SHUTDOWN_EJECT, 
> TRANSLATIONS_UPDATE_DIR, UNSUPPORTED_HW
> +from pyanaconda.constants import SCREENSHOTS_DIRECTORY, 
> SCREENSHOTS_TARGET_DIRECTORY
>  from pyanaconda.regexes import URL_PARSE
>
> from pyanaconda.i18n import _
> @@ -1191,3 +1193,28 @@ def get_platform_groupid():
>  def parent_dir(directory):
>      """Return the parent's path"""
>      return "/".join(os.path.normpath(directory).split("/")[:-1])
>
> +def sysroot_path(path):
> """Make the given relative or absolute path "sysrooted"
>   :param str path: path to be sysrooted
>   :returns: sysrooted path
>   :rtype: str
> """
> return os.path.join(getSysroot(), path.lstrip(os.path.sep))
>
> +def save_screenshots():
> """Save screenshots to the installed system"""
> if not os.path.exists(SCREENSHOTS_DIRECTORY):
>    # there are no screenshots to copy
>    return
> target_path = sysroot_path(SCREENSHOTS_TARGET_DIRECTORY)
> log.info("saving screenshots taken during the installation to:
>
> %s" % target_path)

I think you should use ',' instead of '%'. So this way:

log.info("... installation to: %s", target_path)
> try:
>    # create the screenshots directory
>    mkdirChain(target_path)
>    # copy all screenshots
>    for filename in os.listdir(SCREENSHOTS_DIRECTORY):
>        shutil.copy(os.path.join(SCREENSHOTS_DIRECTORY, 
>
> filename), target_path)
> except OSError:
>    log.exception("saving screenshots to installed system 
>
> failed")

.
.
.
> > diff --git a/pyanaconda/ui/gui/__init__.py 
> > b/pyanaconda/ui/gui/__init__.py
>
> index 9251949..8ffc723 100644
>  from pyanaconda.ui import common
>  from pyanaconda.ui.gui import GUIObject
> -from pyanaconda.ui.gui.utils import gtk_call_once, escape_markup
> +from pyanaconda.ui.gui.utils import gtk_call_once, fire_gtk_action, 
> escape_markup
>
> import logging
>  log = logging.getLogger("anaconda")
> @@ -54,6 +54,8 @@ class Hub(GUIObject, common.Hub):
>         additional standalone screens either before or after them.
>      """
> handles_autostep = True
> def __init__(self, data, storage, payload, instclass):
>        """Create a new Hub instance.
>
> @@ -91,6 +93,9 @@ class Hub(GUIObject, common.Hub):
>      self._checker = None
>
>    self._spokesToStepIn = []
>    self._spokeAutostepIndex = 0
> def _createBox(self):
>        from gi.repository import Gtk, AnacondaWidgets
>        from pyanaconda.ui.gui.utils import setViewportBackground
>
> @@ -353,6 +358,11 @@ class Hub(GUIObject, common.Hub):
>      def spoke_done(self, spoke):
>          spoke.visitedSinceApplied = True
>    # don't apply any actions if the spoke was visited 
>
> automatically
>    if spoke.automaticEntry:
>        spoke.automaticEntry = False
>        return
>    # Don't take visitedSinceApplied into account here.  It will 
>
> always be
>          # True from the line above.
>          if spoke.changed and (not spoke.skipTo or (spoke.skipTo and 
> spoke.applyOnSkip)):
> @@ -384,3 +394,37 @@ class Hub(GUIObject, common.Hub):
>          else:
>              self.main_window.returnToHub()
> def _doAutostep(self):
>    """Autostep through all spokes managed by this hub"""
>    log.info("autostepping through all spokes on hub %s", 
>
> self.__class__.__name__)
>    # created a list of all spokes in reverse alphabetic order, 
>
> we will pop() from it when
>    # processing all the spokes so the screenshots will actually 
>
> be in alphabetic order
>    self._spokesToStepIn = 
>
> list(reversed(sorted(self._spokes.values(), key=lambda x: 
> x.__class__.__name__)))
>    # we can't just loop over all the spokes due to the 
>
> asynchronous nature of GtkStack, so we start by
>    # autostepping to the first spoke, this will trigger a 
>
> callback that steps to the next spoke,
>    # until we run out of unvisited spokes
>    self._autostepSpoke()

Please make this readable by adding some blank lines here.
> def _autostepSpoke(self):
>    if self._spokesToStepIn:
>        spoke = self._spokesToStepIn.pop()
>        self._spokeAutostepIndex += 1
>        log.debug("stepping to spoke %s (%d/%d)", 
>
> spoke.__class__.__name__, self._spokeAutostepIndex, 
> len(self._spokes))
>        spoke.automaticEntry = True
>        spoke.autostepDoneCallback = lambda x: 
>
> self._autostepSpoke()
>        # this is the last spoke, tell it to return to hub once 
>
> the screenshot is taken
>        if self._spokesToStepIn == []:
>            spoke.lastAutostepSpoke = True
>        #fire_gtk_action(self._on_spoke_clicked, None, None, 
>
> spoke)
>        fire_gtk_action(self._on_spoke_clicked, None, None, 
>
> spoke)
>    else:
>        log.info("autostep for hub %s finished", 
>
> self.__class__.__name__)
>        gtk_call_once(self._doPostAutostep)

Same as before some blank lines please.
> def _doPostAutostep(self):
>    if self._spokesToStepIn:
>        # there are still spokes that need to be stepped in
>        return
>    # we are done, re-emit the continue clicked signal we 
>
> "consumed" previously
>    # so that the Anaconda GUI can switch to the next screen (or 
>
> quit)
>    self.window.emit("continue-clicked")

Except the three above everything looks good to me.