On 19 October 2011 13:03, Antoine <r2rien gmail com> wrote: > Le 19/10/2011 00:06, Kai a écrit : > >> > From looking at the patch, there's still some work needed before it >> could actually be merged. For a start, it fails if dbus isn't >> available (yes, it looks like it tries to fail gracefully, but it >> doesn't; declaring DBusProvider requires dbus.service.Object, which >> breaks). Also, Stephen Kennedy's comments from bug 453670 are still >> relevant, and there's an optparse-related bugfix from Jeff Oliver in >> the same bug that isn't incorporated. > > thanks for your time and following up on this > At the time of submitting the patch in > https://bugzilla.gnome.org/show_bug.cgi?id=616466, I was (and still) using > meld inside shell scripts to compare a bunch of files in one shot; thus I > wasn't realising the functionality (and importance) using meld as my vc > merge tool > I sent too much patches in my previous post - you should consider only the > last one > > Since then I re-introduced a real --newtab option consideration wich occur > if not any of your parser.error triggers > Thus, and if I tested well all combinations, the optparse-related bug/bugfix > is no more accurate, and if any parser.error trigger it will not break any > existing instance, dbus registered or not. > > For dbus.service.Object breaking please find attached a new version of the > patch: if dbus not available, when --newtab option is called, a message > stating dbus not supported is printed and meld is launched as if no --newtab > option asked > ...but I'm not sure a class inside a try/except is aproriate > >> The patch also demonstrates the issue of exposing a dbus ABI. The >> patch carries forward the auto-compare option in the dbus method, and >> I'm not sure that I'd want to include that at all. > > I am not sure I understand well the threat of auto-compare when called with > --new-tab... > The use case I see is or you are using meld in combination with vc as a > merge tool or you use it as a gui diff tool and explicitely called as this > with --new-tab you want auto-compare > But, when I might understand better, I am ready to try to find a way you > will agree > >> Looking at these issues is on my list, but as usual, it's a long list. > > sure I understand there might have higher priority issues, > so thanks again for your time and for considering to merge this patch > cheers, > Antoine I spent some time on this, trying to figure out how best to deal with optional D-Bus use, which is awkward. In the end, I settled on what I think is a reasonable scheme, and most of the complexity is in the launcher script and new dbus-service package. I'm attaching a new patch for testing. This is based on your patches, but updated to more recent mainloop integration API (requires dbus-python 0.83), and uses dbus proxies in a different way. This may also make it easier to forward-port to the gdbus bindings eventually. I think that the functionality you proposed is all there, except for the --auto flag. The problem there is that it's confusing that we'd support --auto and not, say, --label. We need to figure out which flags should be carried forward into the dbus API, and add support for those. Probably this just means that OpenPaths will take an additional a{sv} parameter. Feedback and testing very much welcomed. cheers, Kai
Attachment:
0001-Initial-support-for-opening-in-new-tabs-using-D-Bus.patch
Description: Binary data