[PATCH master/rhel7-branch 2/2] Do error checking of repository names on "Installation Source" screen.

David Shea dshea at redhat.com
Fri Oct 18 20:54:10 UTC 2013


On 10/18/2013 02:15 PM, mulhern wrote:
> Resolves #rhel1020991, #fed985103
Our commit hook looks for lines like

Resolves: rhbz#1020991

to check for the ack flags, so switch the Resolves line to that, please. 
And rather than referring to both the Fedora and the RHEL bug in the 
same commit message, we usually edit the message before pushing so that 
rhel7-branch will have 1020991 and master/f20-branch will have 985103. 
This makes Brian's life easier when he goes to make a build.
> *) When the user presses Done, if there are duplicate entries, the repo changes
> are not saved and an error message appears back at the hub. When the user
> returns to the spoke, they are presented with the same list they were looking at
> when they pressed Done.
Should we allow the user to leave the spoke at all? You can override 
on_back_clicked in the spoke and just return if there are errors.

Related, but unhelpful for rhel7-branch since the code is only on f20 
and master, there's some functions that can be used to perform checks as 
input changes and will switch the focus to bad input when Done is 
clicked: GUICheck and add_check in pyanaconda/ui/gui/__init__.py. It's 
only used in the user and password spokes right now, and it might not be 
particularly useful for this situation since it's multiple inputs 
instead of a single entry. Just a thought.

> PyGobject does not handle the editing-done signal properly, so the changed
> signal is the only signal that can be used to detect any changes in the Name
> field. Thus, names are checked, and may be assigned, on every keypress, instead
> of just when the user presses enter. A bug report has been filed against
> PyGobject.
I'm not arguing that it's not a bug, but editing-done is only emitted 
for GtkEntry widgets that are part of a GtkTreeView. This is true for 
gtk itself, not just the pygobject interface.

The most recent thread I can find of people talking about this is a 
decade old, so just get used to using "changed" I guess, sadface: 
https://mail.gnome.org/archives/gtk-app-devel-list/2003-June/msg00035.html
> diff --git a/pyanaconda/ui/gui/spokes/source.glade b/pyanaconda/ui/gui/spokes/source.glade
> index 8b4ed8f..7cc690a 100644
> --- a/pyanaconda/ui/gui/spokes/source.glade
> +++ b/pyanaconda/ui/gui/spokes/source.glade
> @@ -786,11 +786,9 @@
>                           <child>
>                             <object class="GtkButton" id="proxyButton">
>                               <property name="label" translatable="yes">_Proxy setup...</property>
> -                            <property name="use_action_appearance">False</property>
>                               <property name="visible">True</property>
>                               <property name="can_focus">True</property>
>                               <property name="receives_default">True</property>
> -                            <property name="use_action_appearance">False</property>
>                               <property name="use_underline">True</property>
>                               <signal name="clicked" handler="on_proxy_clicked" swapped="no"/>
>                             </object>
> @@ -804,13 +802,11 @@
>                           <child>
>                             <object class="GtkCheckButton" id="mirrorlistCheckbox">
>                               <property name="label" translatable="yes">This URL refers to a _mirror list.</property>
> -                            <property name="use_action_appearance">False</property>
>                               <property name="visible">True</property>
>                               <property name="can_focus">True</property>
>                               <property name="receives_default">False</property>
>                               <property name="no_show_all">True</property>
>                               <property name="hexpand">True</property>
> -                            <property name="use_action_appearance">False</property>
>                               <property name="use_underline">True</property>
>                               <property name="xalign">0</property>
>                               <property name="draw_indicator">True</property>
> @@ -938,6 +934,7 @@
>                       <property name="visible">True</property>
>                       <property name="can_focus">False</property>
>                       <property name="halign">start</property>
> +                    <property name="xalign">0.50999999046325684</property>
>                       <property name="label" translatable="yes">Additional repositories</property>
>                       <attributes>
>                         <attribute name="font-desc" value="Cantarell 10"/>
Like you said, these changes aren't a big deal and are just the result 
of glade having different opinions about things between versions
> @@ -1024,7 +1021,7 @@
>                           <property name="height_request">36</property>
>                           <property name="visible">True</property>
>                           <property name="can_focus">False</property>
> -                        <property name="toolbar_style">icons</property>
> +                        <property name="toolbar_style">text</property>
>                           <property name="show_arrow">False</property>
>                           <property name="icon_size">1</property>
>                           <style>
...but this is a problem. Changing toolbar_style displays the titles of 
the repository buttons instead of the icons.

> diff --git a/pyanaconda/ui/gui/spokes/source.py b/pyanaconda/ui/gui/spokes/source.py
> index 884f201..654dfab 100644
> --- a/pyanaconda/ui/gui/spokes/source.py
> +++ b/pyanaconda/ui/gui/spokes/source.py
> @@ -25,7 +25,7 @@ import time
>   import logging
>   log = logging.getLogger("anaconda")
>   
> -import os.path
> +import os.path, string
>   
>   # pylint: disable-msg=E0611
>   from gi.repository import AnacondaWidgets, GLib, Gtk
> @@ -277,6 +277,7 @@ class SourceSpoke(NormalSpoke):
>           self._error = False
>           self._proxyChange = False
>           self._cdrom = None
> +        self._rejected_repos = False
>   
>       def apply(self):
>           # If askmethod was provided on the command line, entering the source
> @@ -463,7 +464,7 @@ class SourceSpoke(NormalSpoke):
>           if flags.automatedInstall and (not self.data.method.method or not self.payload.baseRepo):
>               return False
>           else:
> -            return not self._error and self.ready and (self.data.method.method or self.payload.baseRepo)
> +            return not self._error and not self._rejected_repos and self.ready and (self.data.method.method or self.payload.baseRepo)
>   
>       @property
>       def mandatory(self):
> @@ -484,7 +485,11 @@ class SourceSpoke(NormalSpoke):
>               return _("Checking software dependencies...")
>           elif not self.ready:
>               return _(BASEREPO_SETUP_MESSAGE)
> -        elif self._error or not self.payload.baseRepo:
> +        elif not self.payload.baseRepo:
> +            return _("Error setting up base repository")
> +        elif self._rejected_repos:
> +            return _("Duplicate repository names specified")
> +        elif self._error:
>               return _("Error setting up software source")
>           elif self.data.method.method == "url":
>               return self.data.method.url or self.data.method.mirrorlist
> @@ -494,7 +499,7 @@ class SourceSpoke(NormalSpoke):
>               return _("CD/DVD drive")
>           elif self.data.method.method == "harddrive":
>               if not self._currentIsoFile:
> -                return _("Error setting up software source")
> +                return _("Error setting up ISO file")
>               return os.path.basename(self._currentIsoFile)
>           elif self.payload.baseRepo:
>               return _("Closest mirror")
> @@ -556,6 +561,14 @@ class SourceSpoke(NormalSpoke):
>           else:
>               really_hide(self._updatesBox)
>   
> +        self._repoNameWarningBox = self.builder.get_object("repoNameWarningBox")
> +        self._repoNameWarningLabel = self.builder.get_object("repoNameWarningLabel")
> +        really_hide(self._repoNameWarningBox)
> +
> +        self._repoNamesWarningBox = self.builder.get_object("repoNamesWarningBox")
> +        self._repoNamesWarningLabel = self.builder.get_object("repoNamesWarningLabel")
> +        really_hide(self._repoNamesWarningBox)
> +
>           threadMgr.add(AnacondaThread(name=constants.THREAD_SOURCE_WATCHER, target=self._initialize))
>   
>       def _initialize(self):
You can leave out the really_hide calls here, since the glade already 
makes the two boxes initially invisible.

All the rest looks ok to me.


More information about the anaconda-patches mailing list