On 07/15/2012 08:53 AM, Scott Talbert wrote: > Hi Stephen, > > I have been contributing to Concordance lately, and I have written a > patch for Congruity to port it to the new libconcord API. If you have > some time, I would greatly appreciate it if you could review the patch, > which I have attached. Details on my changes are listed below.
Sorry for the slow response; I've been away on vacation. I've quoted the attachment below. A lot of my feedback is more about the libconcord API changes than the congruity patch itself. > +import threading It'd be better to simply consistently use thread rather than mixing thread and threading. > -version = "15+" > +version = "16+" Functional patches shouldn't touch the version; it should only be bumped during or right after release. > -# Fix typo in libconcord 0.20 Python bindings > +# Test to ensure we have the new API available. > try: > - libconcord.delete_blob > + libconcord.update_configuration > except: > - libconcord.delete_blob = libconcord.delete_block > + str = traceback.format_exc() > + app = wx.PySimpleApp() > + dlg = wx.MessageDialog( > + None, > + "Could not load the correct version of libconcord; please ensure " > + "that the latest version is installed.\n\n" + str, > + "congruity: Dependency Error", > + wx.OK | wx.ICON_ERROR > + ) > + dlg.ShowModal() > + os._exit(1) Hmm. I've made sure that congruity to date can run with almost any version of libconcord. That change breaks this feature. Isn't there a way to introduce support for the new libconcord APIs without making it non-backwards-compatible? > -def program_callback_imp(count, current, total, context): > +def program_callback_imp(stage_id, count, current, total, type, context): > if not context: > return > > + if stage_id == libconcord.LC_CB_STAGE_NUM_STAGES: > + return > + > try: > - (f, fcontext) = context > - percent = (current * 100) / total > - f(False, percent, fcontext) > + if isinstance(context, ProgramRemotePanelBase): That's not very object-oriented; it'd be better to call a function/method in the context object, and have that function do whatever is appropriate, rather than having the code query the type of the context and then act differently. > + if stage_id not in context.dg_widgets: > + context.dg_widget_added = threading.Event() > + wx.CallAfter(context._AddDg, stage_id) > + context.dg_widget_added.wait() > + context._DgStart(context.dg_widgets[stage_id]) Uggh. I'll comment on that later. > @@ -379,54 +365,7 @@ class WelcomePanel(MessagePanelBase): > > self.next = self.resources.page_connect > > - xml = POINTER(c_ubyte)() > - xml_size = c_uint() > - libconcord.read_file( > - self.resources.ezhex_filename, > - byref(xml), > - byref(xml_size) > - ) > - self.resources.SetXmlData(xml, xml_size) > - > - type = c_int() > - libconcord.identify_file(xml, xml_size, byref(type)) Uggh. Why can't libconcord parse the file without being attached to the remote; the file format should be completely standalone and parse-able "offline". > @@ -688,7 +694,6 @@ class ProgramRemotePanelBase(WizardPanelBase, Deco > > self.dc = DecoratedContainer(self, self.resources) > DecoratedContainerThreadMixin.__init__(self, self.dc) > - self._AddWidgets() > self.sizer.Add(self.dc, 0, wx.EXPAND | wx.ALL, 5) This and the related changes will cause a horrible regression in the utility of the GUI; the poor user will just see more and more tasks randomly appearing, and have no idea when the end is in sight. congruity may as well embed a vte (terminal) widget and just run the concordance application in it given this change. I've complained about this libconcord API change repeatedly before... That all said, I don't really have much time or motivation for congruity work these days, so I'm not going to really push back hard on these changes. If you want, perhaps I can hand off congruity maintenance to you since you've obviously got some interest in it; I assume that Sourceforge will let me set up other committers/admins. Let me know if you want, and what your Sourceforge login ID is. ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
