[Sugar-devel] [PATCH] Downgrading activities not allowed. (#2164)

[James Cameron]

Nak.  See detailed comments below.

On Tue, Oct 12, 2010 at 01:15:34AM +0530, shanjit at seeta.in wrote:
> From: Shanjit Singh Jajmann <shanjit at seeta.in>
> 
> Downgrading an activity is now made possible. When a .xo file of a version older than the currently installed version is clicked, a downgrading option is made available, by popping up of a confirmation alert. Depending upton the choice selected you can downgrade the activity.

Please text wrap these subsequent lines of your commit message.

>  mode change 100644 => 100755 src/jarabe/journal/journalactivity.py
>  mode change 100644 => 100755 src/jarabe/journal/misc.py
>  mode change 100644 => 100755 src/jarabe/model/bundleregistry.py

You are incorrectly changing file modes.  Please restore the file modes
before you commit.

> @@ -128,7 +129,7 @@ class JournalActivity(Window):
>          self.connect('window-state-event', self.__window_state_event_cb)
>          self.connect('key-press-event', self._key_press_event_cb)
>          self.connect('focus-in-event', self._focus_in_event_cb)
> -
> +        

Please remove this gratuitous whitespace change from your patch.

> @@ -145,7 +145,30 @@ class JournalActivity(Window):
>          alert.connect('response', self.__alert_response_cb)
>          self.add_alert(alert)
>          alert.show()
> -
> +        
> +    def __activity_alert1_cb(self):

Please use a method name that describes the action to be taken.  As
Bernie pointed out, the name you used did not explain the reason for the
alert.

> +	    if misc.check_previous_install() == 1 and misc.return_checked()==0:
> +             alert1 = ConfirmationAlert()
> +             logging.debug('value of misc is %d', misc.check_previous_install())
> +             alert1.props.title=_('Previous Version Found')
> +             alert1.props.msg = _('A previous version of an installed activity was found. Are you sure you want to continue with installation ?                                                                      If Yes click Ok and the activity icon of the older .xo file in the Journal')

The message you have written is ambiguous and could be read in either of
two ways; that the user has clicked on a newer version, or that the user
has clicked on an older version.

> +    def __alert1_response_cb(self, alert1, response_id):
> +        if response_id is gtk.RESPONSE_OK:
> +            logging.debug('value of checked initial %d', misc.return_checked())
> +            logging.debug('Installing previous version')
> +            self.remove_alert(alert1)
> +            misc.checked = 1
> +            logging.debug('value of checked final %d', misc.return_checked())
> +            
> +        elif response_id is gtk.RESPONSE_CANCEL:
> +            logging.debug('Cancelled by user')
> +            self.remove_alert(alert1)

The logging is useful to you at this stage of development, but you
should assess whether it is useful to other developers.  Perhaps some of
these and later logging calls should be removed.

> @@ -148,31 +152,53 @@ def get_activities(metadata):
> [...]
>  def resume(metadata, bundle_id=None):
>      registry = bundleregistry.get_registry()
> -
> +    global checker
> +    global checked

I agree with Bernie ... there seems to be no justification for using
globals in this situation.

-- 
James Cameron
http://quozl.linux.org.au/