Merge lp:~gary-lasker/oneconf/wire-app-action into lp:oneconf

Proposed by Gary Lasker
Status: Merged
Merged at revision: 73
Proposed branch: lp:~gary-lasker/oneconf/wire-app-action
Merge into: lp:oneconf
Diff against target: 11 lines (+1/-0)
1 file modified
oneconf/usc_plugin.py (+1/-0)
To merge this branch: bzr merge lp:~gary-lasker/oneconf/wire-app-action
Reviewer Review Type Date Requested Status
Didier Roche-Tolomelli Approve
Review via email: mp+31660@code.launchpad.net

Description of the change

Hello didrocks!

I had some time late yesterday to take a quick look at OneConf and I have a fix for the Install/Remove buttons not working bug. This should be all you need (I tested and it appears to do the trick, please let me know if any problems).

Oh, and by the way, we will be moving to using gtk.Action for implementing the package actions rather than the current signals, but it's currently a little tricky because of our custom views/button implementations, etc. -- however, once we go to that this should be a little cleaner for you.

One thing about this fix, if we will be getting refresh_hosts and creating new instances of OneConfPane, we'll want to detach the "application-request-action" listener from the old pane when we create the new one, else the old one won't get garbage-collected. Let me know if that's the case and I'll make that change.

I think you also had a problem with show/hide apps not working. I'll try to take a look at that one asap. Are there any other remaining issues you'd like me to look at?

Thanks!
Gary

To post a comment you must log in.
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Thanks a lot for the fix!

well, I think we can keep it this way for now. I don't think we will destroy new panes during USC lifecycle, so it's safe to keep it this way, isn't it?
(we just create a default set of pane as you saw in the for loop, and then, can create more on the fly and hide some). Will we get still some garbage-collector issue?
gtk.Action sounds good. I'm looking forward to it!

Great, you patch is working (at least, with one pane). Merging it now.

For your question, yeah, the show/hide apps is the remaining things that needs work!
Thanks again!

review: Approve
Revision history for this message
Gary Lasker (gary-lasker) wrote :

Cool! Yeah, I didn't look closely enough so I had assumed you were dropping the old instance of the OneConf pane when you made a new one -- I didn't realize you were just making more. So yes, in that case, we should be good to go!

Thanks Didier!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'oneconf/usc_plugin.py'
2--- oneconf/usc_plugin.py 2010-08-02 14:30:01 +0000
3+++ oneconf/usc_plugin.py 2010-08-03 16:04:47 +0000
4@@ -107,6 +107,7 @@
5 if current_hostid not in self.view_switchers_oneconf_hostid:
6 current_pane = oneconfpane.OneConfPane(self.app.cache, None, self.app.db, 'Ubuntu', self.app.icons, self.app.datadir, self.oneconfeventhandler, current_hostid)
7 self.app.view_manager.register(current_pane, current_hostid)
8+ current_pane.app_view.connect("application-request-action", self.app.on_application_request_action)
9 icon = AnimatedImage(view_switcher.icons.load_icon("computer", model.ICON_SIZE, 0))
10 current_iter = model.insert_after(None, previous_iter, [icon, new_elem[current_hostid], current_hostid, channel])
11 previous_iter = current_iter

Subscribers

People subscribed via source and target branches