Sir, Regards. <div class="gmail_quote">On Mon, Apr 30, 2012 at 7:38 PM, Sascha Silbe <<a href="mailto:silbe@activitycentral.com" target="_blank">silbe@activitycentral.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Excerpts from Ajay Garg's message of 2012-04-26 23:53:51 +0200: <div class="im"> > The feature specs, and workflow-screenshots, are listed at :: > <a href="http://wiki.sugarlabs.org/go/Features/Transfer_to_many_options" target="_blank">http://wiki.sugarlabs.org/go/Features/Transfer_to_many_options</a> </div>This patch is HUGE. Please split it into several patches,</blockquote><div> The patch will be split, once the files at "src/webdav/*" and "src/webdav/acp/*" will be packaged into a rpm. Actually, I had to made two changes in src/webdav/WebdavResponse.py, so that the "GetLastModified()" and "GetCreationDate()" return the values in timestamp format (in conformance with what is required in listview). Perhaps, we should then make a (separate) git repo of it. Let me sleep over this for a while :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> based on functional changes. The cover letter should explain the architecture of your solution (e.g. using a third-party WebDAV client library rather than glib / gio) and why you chose it. </blockquote><div> Done. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [...] > create mode 100644 src/webdav/Condition.py [...] This looks like an embedded copy of some external component. Never do that. Even if we had the resources to maintain the embedded copy (updating it whenever upstream publishes a new version), its history would be needlessly mingled with that of Sugar and we'd need to release the combination when either of the parts gets updated. The current structure of Sugar is enough of a mess already, we don't need to add to that. If the library isn't packaged for most of the major distros, we should consider whether it provides significant advantage over the alternatives and if - after that consideration - we still want to use it, try to get it into distros. If that fails, we can build a custom package for Dextrose. Bundling it with Sugar, however, is not a useful option. </blockquote><div> Yep, same feeling here. So, a rpm will be formed. </div><div> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [<a href="http://configure.ac" target="_blank">configure.ac</a>] <div class="im">> @@ -61,8 +61,6 @@ extensions/cpsection/modemconfiguration/config.py > extensions/cpsection/Makefile > extensions/cpsection/network/Makefile > extensions/cpsection/power/Makefile > -extensions/cpsection/updater/backends/Makefile > -extensions/cpsection/updater/Makefile </div>What does WebDAV support have to do with the software update CP section? </blockquote><div> Well, I had removed this, since sugar-jhbuild wasn't compiling otherwise. Tested on an image with this change, "Software Update" worked fine. </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [src/jarabe/journal/journalactivity.py] <div class="im">> @@ -485,6 +485,9 @@ class JournalActivity(JournalWindow): > def is_editing_mode_present(self): > return self._editing_mode > > + def get_volumes_toolbar(self): > + return self._volumes_toolbar </div>Giving some other component access to an internal part of the Journal Activity is usually going to be the wrong approach. Consider adding appropriate API to do only what you need. Depending on what that is, possible approaches may be: 1. Add API to the other component and let the Journal Activity or the Volumes Toolbar take action based on that information (e.g. deactivate some buttons for read-only mounts). 2. Add a signal source to the other component and let the Journal Activity or the Volumes Toolbar subscribe ("connect") to it. Useful if the information based on which the Journal Activity needs to base its decision can change at any time. 3. Add API to the Journal Activity to pass through information from the Volumes Toolbar so that the other component can query it. It's worth considering why you are asking the Journal Activity, though. It's the top-level component and should connect the pieces, not usually be queried by one of the pieces. 4. Add a signal source to the Journal Activity to broadcast information needed by the other component. Similar to 3., but can be used for information that can change at any time. </blockquote><div> Well, I am reluctant to do this, primarily because the signal-mechanism is very messy (as far as code-browsing is concerned). I remember I had to spend quite too much time for finding what ultimately happens with the 'volume-error' signal (think there were about 5 levels of re-direction). Moreover, I think that that 'signals' are more suited in situations when the 'cause' component performs some action asynchronously (which is not the case here; we just need to perform post-actions synchronously, as and when the 'cause' action happens). Another thing, that 'JournalActivity' and its 'volumestoolbar' constiturent are on a singleton basis; so we do not need to worry about mangling different instances :) </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> [src/jarabe/journal/journaltoolbox.py] <div class="im">> @@ -624,6 +624,13 @@ class BatchEraseButton(ToolButton, palettes.ActionItem): > show_not_completed_ops_info=True) > self.props.tooltip = _('Erase') > > + # De-sensitize Batch-Erase button, for locally-mounted-remote-shares. > + from jarabe.journal.journalactivity import get_mount_point > + current_mount_point = get_mount_point() > + > + if model.is_mount_point_for_locally_mounted_remote_share(current_mount_point): > + self.set_sensitive(False) </div>Similar issue here. Connecting to a signal that broadcasts the change of the active mount point may be a good choice, but make sure to put it at a reasonable level, not in the individual button. I'll stop here for now; will probably wait for the next revision before commenting on the rest of the changes. Sascha -- <a href="http://sascha.silbe.org/" target="_blank">http://sascha.silbe.org/</a> <a href="http://www.infra-silbe.de/" target="_blank">http://www.infra-silbe.de/</a> </blockquote><div> Regards, Ajay </div></div>