[Sugar-devel] [DESIGN] Proposal: Multi-Selection and Batch Operations on Journal entries

[Ajay Garg]

Hi Simon.

Thanks for the reply !!

Please find my comments inline.


On Tue, Aug 28, 2012 at 3:13 PM, Simon Schampijer <simon at schampijer.de>wrote:

> On 08/19/2012 10:05 PM, Ajay Garg wrote:
>
>> Hi all.
>>
>> Just saw Simon's patch at http://patchwork.sugarlabs.**org/patch/1670/<http://patchwork.sugarlabs.org/patch/1670/>
>>
>> Thereby, it's a gentle request ::
>>
>>                    * to get this multi-select feature approved (with all
>> the
>> latest feedback-changes/bug-fixes).
>>                    * to get the multi-select patch(es) reviewed.
>>                    * to get the multi-select patch(es) pushed (if all ok).
>>
>>
>> so that any such enhancements (confirmation alert before erasing,
>> currently
>> in single-mode) can be easily merged (for single- AND batch-mode); and
>> there is minimal slogging involved that comes in with merging patches :)
>>
>>
>> Thanks and Regards,
>> Ajay
>>
>
>
>
> Hi Ajay,
>
> first of all thanks for working on this. Yes, Sugar is moving fast those
> days, you can apply your patch on top of 3aa88a85143c48b64cf57ab7c79b45**31dcfa6e8b
> (Release 0.97.1). You can keep working on top of that and rebase later so
> you don't have to keep up with master. In any case, you need to port them
> to gtk3 anyhow later.
>

Okies..





>
> First of all, I think it would be good to send the patches directly to the
> devel list (as for all I know they are currently only in patchwork) [1] .
> If I missed anything, please point me to it.
>

I had already sent the patches to sugar-devel earlier :) at ::

http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039054.html
http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039055.html
http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039056.html


As per your suggestion, I will be changing, and soon posting the changed,
version-2 sugar-patch (replying on that same thread), that would do away
with the need of the sugar-toolkit patch :)






>
> Please add detailed descriptions to the patch (see how I tried and
> hopefully managed to describe a big change we lately landed in [2]). Get to
> the point what the patch does. Add items that don't work yet or are
> critical or if you have questions or are unsure about the approach.
>

Ok, I think the bulk of the work happens in the different, segregated out
stages during a particular metadata-processing. I took care of being
verbose there as far as possible :)

Anyhow, in my version-2 patch (that would do away with the need of the
sugar-tollkit patch, as pointed by you), I will also try my hand to have a
better, unified description :)






>
> The good news is, the artwork and toolkit patches are easy to handle. The
> icons should be cleaned up and can be landed. For the toolkit patch I think
> you can use add_button fine:
>
> diff --git a/src/jarabe/journal/**journalactivity.py b/src/jarabe/journal/
> **journalactivity.py
> index 7ed8b18..060416b 100644
> --- a/src/jarabe/journal/**journalactivity.py
> +++ b/src/jarabe/journal/**journalactivity.py
> @@ -132,16 +132,17 @@ class JournalActivity(JournalWindow)**:
>          self._editing_mode = False
>          self._alert = Alert()
>
> -        self._error_alert = NButtonAlert()
> -        self._error_alert._populate_**buttons([
> -            ('dialog-ok', gtk.RESPONSE_OK, _('Ok'))
> -                                            ])
> -
> -        self._confirmation_alert = NButtonAlert()
> -        self._confirmation_alert._**populate_buttons([
> -            ('dialog-cancel', gtk.RESPONSE_CANCEL, _('Stop')),
> -            ('dialog-ok', gtk.RESPONSE_OK, _('Continue'))
> -                                                   ])
> +        self._error_alert = Alert()
> +        icon = Icon(icon_name='dialog-ok')
> +        self._error_alert.add_button(**gtk.RESPONSE_OK, _('Ok'), icon)
> +
> +        self._confirmation_alert = Alert()
> +        icon = Icon(icon_name='dialog-cancel'**)
> +        self._confirmation_alert.add_**button(gtk.RESPONSE_CANCEL,
> +                                            _('Stop'), icon)
> +        icon = Icon(icon_name='dialog-ok')
> +        self._confirmation_alert.add_**button(gtk.RESPONSE_OK,
> +                                            _('Continue'), icon)
>
> We could argue that the add_button API should allow to pass in an icon
> name instead of the icon directly, I don't think there is a case where this
> would break. On the other hand not sure changing that now. I would just go
> with the above and drop your toolkit patch.
>

+1.





>
> So we are left with the shell patch basically. It is loong and invasive :)
> So it will take some time. More review eyes welcome. I wonder as well if
> splitting it up in sub-patches would help, but maybe is too hard at this
> point.
>

Hmm.. Yes, there is a tendency for things to break.
I will be posting with a better, unified description in version-2 patch.



>
> On the design end, the work flow is good. I guess the 'uifreeze' is meant
> to prevent races.



+1.
Gary's aggressive testing produced plenty of those :D

So, freezing the UI was the only (and actually, simple and correct)
approach.




> Is the first time we do it in the UI. Might make sense here. I will think
> about it a bit more.
>

> Cheers,
>    Simon
>
> [1] http://wiki.sugarlabs.org/go/**Development_Team/Code_Review<http://wiki.sugarlabs.org/go/Development_Team/Code_Review>
> [2] http://git.sugarlabs.org/**sugar/mainline/commit/**
> d9fbf9db79b9abab2491ebbdc16998**5e38a59055<http://git.sugarlabs.org/sugar/mainline/commit/d9fbf9db79b9abab2491ebbdc169985e38a59055>
>
> PS: something that caught my eye in journalactivity.py:
> {{{
>  def __volume_error_cb(self, gobject, message, severity):
>         alert = ErrorAlert(title=severity, msg=message)
>         alert.connect('response', self.__alert_response_cb)
>         self.add_alert(alert)
>         alert.show()
>
>     def _volume_error_cb(self, gobject, message, severity):
>         self.update_error_alert(**severity, message, None, None)
> }}}
>

This would be called in the "journal._volume_error_cb(menu_item, message,
severity)" in "CopyMenuHelper" class in "src/jarabe/journal/palettes.py".
Since "CopyMenuHelper" is a singleton class, and is NOT attached anywhere
to the singleton "JournalActivity" widget, thus, there is no way to call
the private "__volume_error_cb" method of "JournalActivity" widget directly.




Thanks and Regards,
Ajay
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120828/c72aecfb/attachment-0001.html>