Hi Simon. Thanks for the reply !! Please find my comments inline. <div class="gmail_quote">On Tue, Aug 28, 2012 at 3:13 PM, Simon Schampijer <<a href="mailto:simon@schampijer.de" target="_blank">simon@schampijer.de</a>> wrote: <blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">On 08/19/2012 10:05 PM, Ajay Garg wrote: <blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Hi all. Just saw Simon's patch at <a href="http://patchwork.sugarlabs.org/patch/1670/" target="_blank">http://patchwork.sugarlabs.org/patch/1670/</a> 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 </blockquote> </div> 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 3aa88a85143c48b64cf57ab7c79b4531dcfa6e8b (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. </blockquote><div> Okies.. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div> I had already sent the patches to sugar-devel earlier :) at :: <a href="http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039054.html">http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039054.html</a> <a href="http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039055.html">http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039055.html</a> <a href="http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039056.html">http://lists.sugarlabs.org/archive/sugar-devel/2012-August/039056.html</a> 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 :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div> 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 :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div> +1. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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. </blockquote><div> Hmm.. Yes, there is a tendency for things to break. I will be posting with a better, unified description in version-2 patch. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> On the design end, the work flow is good. I guess the 'uifreeze' is meant to prevent races.</blockquote><div> +1. Gary's aggressive testing produced plenty of those :D So, freezing the UI was the only (and actually, simple and correct) approach. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Is the first time we do it in the UI. Might make sense here. I will think about it a bit more. </blockquote><div></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Cheers, Simon [1] <a href="http://wiki.sugarlabs.org/go/Development_Team/Code_Review" target="_blank">http://wiki.sugarlabs.org/go/Development_Team/Code_Review</a> [2] <a href="http://git.sugarlabs.org/sugar/mainline/commit/d9fbf9db79b9abab2491ebbdc169985e38a59055" target="_blank">http://git.sugarlabs.org/sugar/mainline/commit/d9fbf9db79b9abab2491ebbdc169985e38a59055</a> 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) }}} </blockquote><div> 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. </div></div> Thanks and Regards, Ajay