Code review comment for lp://staging/~sylvain-pineau/checkbox/fix-1227684

Revision history for this message
Zygmunt Krynicki (zyga) wrote :

First of all, let's fix the argument name, call it 'suggestion', maybe or make a better name.

Then passing suggestions is *okay* but why are we passing SKIP as a suggestion? I know the answer, because the GUI is limited and that's how it works (it needs that signal so we send it even though it makes no sense).

Can we please try to keep the silliness out of the backend code and keep it in the UI, and remove it as we go?

How about you add a signal ShowInteractiveUI() instead of that AskForOutcome(self, SKIP) and treat it just the same for now. This will, at least, make the dbus API explicit and saner and hide reliance on quirky UI behavior. Then the AskForOuntcome() signal can send the suggewstion as you do now.

I'm a bit worried that it's too generic though, only commands where the return value means something should send the suggestion. We should explicitly enable that, otherwise we'll get trigger-happy "PASS" for commands that echo "this has failed" with exit code 0 from echo.

« Back to merge proposal