[Firstaidkit-devel] Re: [PATCHes] Add question support

[Miloslav Trmac]

Hello,
thanks for the review.

----- "Martin Sivak" <msivak at redhat.com> wrote:
> I have some more "flaws" to discuss:
> 
> + ANSWER = 999#In question reply, {"question", "answer"}
> 
> This says nothing about the type of the answer, the response should
> use common info, tree or table type to create the answer
The type is "a single Python value" - see e.g. the example dialogue plugin, which uses True and False as values.  Wrapping it in any of the TREE/TABLE structures does not provide any additional type information; using INFO would confuse the value with an user-readable error message.

> Also you should use the standard message structure for passing the
> information around, so answer goes to "message" and at least the
> origin field is set.
Fixed.
>
> +    @staticmethod
> +    def __blocking_question(fn, args, kwargs):
> +        queue = Queue.Queue()
> +        question = fn(queue, *args, **kwargs)
> +        r = queue.get()
> +        assert r["action"] == ANSWER and r["question"] is question
> +        return r["answer"]
> 
> You should use openMailbox and closeMailbox for queue management.
The "reply" field documentation required using a Queue.Queue; I have fixed both the code and documentation.

> +    def __init__(self, queue, importance = logging.INFO, secondary =
> False
> 
> Better name for the secondary argument would be "interactive" with
> default value of True
Fixed.


Attached is an updated version of the patch, in addition to the above it also:
* adds Cancel buttons to all GTK dialogs
* allows CHOICE_QUESTION return value to be None
Note that it depends on the "Misc. bug fixes, part 1" patches.
    Mirek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-question-support.patch
Type: application/mbox
Size: 136161 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/firstaidkit-devel/attachments/20090717/05372497/attachment.mbox>