Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I instantiate GtkInfobars a lot in my GTK+ application in order to communicate with the user. There are various types of infobars, depending on the message. Basically, any infobar could be a combination of the 4 different infobar message types and 5 different icons (which are painted on the left side of the infobar).

Initially, I would call my custom infobar function like this:

infobar("Message. Woo.", type=gtk.MESSAGE_INFO, icon=gtk.STOCK_DIALOG_WARNING, timeout=5)

After a while I decided I wanted to simplify all the creation calls... so I modified my infobar function so that I could do this:

infobar("Message. Woo.", type=(1,3), timeout=3)

I feel like the second way is better.. and that it's worth the code obfuscation, but I suspect not everyone will agree with me. So here's the code of my infobar function as it is now. I'd love to get some feedback. Thanks for reading!

NOTE: Skip to the newer edit below.

def infobar(self, msg=None, type=(1,1), timeout=3, vbox=None):
    """Popup a new auto-hiding InfoBar."""

    # List of possible infobar message types
    msgtypes = [0,
                gtk.MESSAGE_INFO,      # 1
                gtk.MESSAGE_QUESTION,  # 2
                gtk.MESSAGE_WARNING,   # 3
                gtk.MESSAGE_ERROR]     # 4

    # List of possible images to show in infobar
    imgtypes = [gtk.STOCK_APPLY,            # 0
                gtk.STOCK_DIALOG_INFO,      # 1
                gtk.STOCK_DIALOG_QUESTION,  # 2
                gtk.STOCK_DIALOG_WARNING,   # 3
                gtk.STOCK_DIALOG_ERROR]     # 4

    ibar                    = gtk.InfoBar()
    ibar.set_message_type   (msgtypes[type[0]])
    if vbox:
        # If specific vbox requested: assume ibar for filemode, add cancel button
        ibar.add_button     (gtk.STOCK_CANCEL, gtk.RESPONSE_CANCEL)
        ibar.connect        ('response', self.cleanup_filemode)
    else:
        # If no specific vbox requested: do normal ibar at the top of message area
        vbox = self.vbox_ibar
        ibar.add_button     (gtk.STOCK_OK, gtk.RESPONSE_OK)
        ibar.connect        ('response', lambda *args: ibar.destroy())
    vbox.pack_end           (ibar, False, False)
    content                 = ibar.get_content_area()
    img                     = gtk.Image()
    img.set_from_stock      (imgtypes[type[1]], gtk.ICON_SIZE_LARGE_TOOLBAR)
    content.pack_start      (img, False, False)
    img.show                ()
    if msg:
        # If msg was specified, show it, but change the default color
        label               = gtk.Label()
        label.set_markup    ("<span foreground='#2E2E2E'>{}</span>".format(msg))
        content.pack_start  (label, False, False)
        label.show          ()
    # FIXME: Why doesn't Esc trigger this close signal?
    ibar.connect            ('close', lambda *args: ibar.destroy())
    ibar.show()
    if timeout:
        glib.timeout_add_seconds(timeout, ibar.destroy)
    return ibar

----------

EDIT:

So Winston has been giving me some ideas. I'm trying out something new. First, I'm storing all of the messages and associated flags in a separate file (messages.py) as a 2d dictionary. Here's an excerpt:

DEFAULT_TIMEOUT = 5
SUCCESS         = 0
INFO            = 1
QUESTION        = 2
WARNING         = 3
ERROR           = 4

MESSAGE_DICT = dict(

    engine_openssl_missing = dict(
        text="<b>Shockingly, your system does not appear to have OpenSSL.</b>",
        type=INFO,
        icon=WARNING,
        timeout=DEFAULT_TIMEOUT
        ),
    txtview_fileopen_error = dict(
        text="<b>Error. Could not open file:\n<i><tt><small>{filename}</small></tt></i></b>",
        type=WARNING,
        icon=ERROR,
        timeout=DEFAULT_TIMEOUT
        ),
    msgbuff_empty = dict(
        text="<b>{customtext}</b>",
        type=INFO,
        icon=WARNING,
        timeout=2
        )
    )

Currently I import that like: from messages import MESSAGE_DICT.

And then here's my new (for the moment) infobar function:

def infobar(self, id, filename=None, customtext=None, vbox=None):
    """Popup a new auto-hiding InfoBar."""

    # Find the needed dictionary inside our message dict, by id
    MSG = MESSAGE_DICT[id]
    # Use value from MSG type & icon to lookup Gtk constant, e.g. gtk.MESSAGE_INFO
    msgtype = MSGTYPES[ MSG['type'] ]
    imgtype = IMGTYPES[ MSG['icon'] ]
    # Replace variables in message text & change text color
    message = ("<span foreground='#2E2E2E'>" +
               MSG['text'].format(filename=filename, customtext=customtext) +
               "</span>")

    # Now that we have all the data we need, START creating!
    ibar = gtk.InfoBar()
    ibar.set_message_type(msgtype)
    if vbox:
        # If specific vbox requested: assume ibar for filemode, add cancel button
        ibar.add_button (gtk.STOCK_CANCEL, gtk.RESPONSE_CANCEL)
        ibar.connect    ('response', self.cleanup_filemode)
    else:
        # If no specific vbox requested: do normal ibar at the top of message area
        vbox            = self.vbox_ibar
        ibar.add_button (gtk.STOCK_OK, gtk.RESPONSE_OK)
        ibar.connect    ('response', lambda *args: ibar.destroy())
    vbox.pack_end       (ibar, False, False)
    content             = ibar.get_content_area()
    img                 = gtk.Image()
    img.set_from_stock  (imgtype, gtk.ICON_SIZE_LARGE_TOOLBAR)
    content.pack_start  (img, False, False)
    img.show            ()
    label               = gtk.Label()
    label.set_markup    (message)
    content.pack_start  (label, False, False)
    label.show          ()
    # FIXME: Why doesn't Esc trigger this close signal?
    ibar.connect        ('close', lambda *args: ibar.destroy())
    ibar.show()
    if MSG['timeout'] > 0:
        glib.timeout_add_seconds(MSG['timeout'], ibar.destroy)
    return ibar

Note that MSGTYPES and IMGTYPES are lists that I moved out of the function, but other than the name change, they're the same as in my original post.

Oh and finally, the way I call the infobars now is, e.g.:

self.infobar('engine_openssl_missing')
self.infobar('txtview_fileopen_error', filename)
self.infobar('msgbuff_empty', customtext="No text to save")

By the way, you can see all this in living color on github. The infobar funness is part of a gui encryption application called Pyrite.

share|improve this question
    
What do you think you've saved by doing this? I.e. to your mind, what's the purpose of being concise? –  Winston Ewert Feb 27 '12 at 15:33
    
@WinstonEwert: Hello again. Well, I call this function a lot.. with multiline messages containing custom .format() substitution and all combinations of the 5 icons, 4 message types, and various timeouts. It just makes the code much simpler-looking, not to mention makes it easier to stick to Pythonic rules of line-width. But seeing the reaction from you and the one answerer... –  ryran Feb 27 '12 at 20:25
    
@ryan, I think you are seeing a valid problem but I'm not sure you've solved it in the best way. Can you share more example of calling the function that show how its giving you trouble? –  Winston Ewert Feb 27 '12 at 21:47
    
@Winston: You could search for self.infobar on this page to see all cases where it's called, but I'm gonna change it. It's not really giving me trouble--I mean.. I guess I saw a problem (and came up with a crazy "solution") where there wasn't really a problem. –  ryran Feb 27 '12 at 22:42
add comment

2 Answers

up vote 3 down vote accepted

I'd move all of those message into an external resource file something like:

<message id="file.open.fail" type="warning" icon="error" delay="5">
    <b>Error. Could not open file:<i><tt><small>{filename}</small></tt></i></b>
</message>

Then I'd simply pop the message using:

self.infobar('file.open.fail', filename = filename)

That should cleanup the calls to infobar, and make it easier to do things like provide strings for a different language.

EDIT:

Quick hack of an implementation

from xml.etree.ElementTree import XML, tostring

# of course, you'd want to load this from an external file
message_file = XML("""
<messages>
    <message id="file.open.fail" type="warning" icon="error" delay="5">
        <b>Error. Could not open file:<i><tt><small>{filename}</small></tt></i></b>
    </message>
    <message id="file.open.good" type="warning" icon="error" delay="5">
        <b>File opened!</b>
    </message>
</messages>
""")

messages = {}
for message in message_file.findall("message"):
    messages[message.attrib['id']] = message

def message(id, **kwargs):
    message_detail = messages[id]
    text = ''.join( tostring(node) for node in message_detail)
    return text.strip().format(**kwargs)

print message('file.open.fail', filename = 'file.txt')
print message('file.open.good', filename = 'file.txt')

Doesn't try to handle type/icon/delay. You'll probably also want to consider how you want to do whitespace.

EDIT (NEW VERSION):

default_timeout = 5
msgs = dict(

Python convention is to put the names for global constants like these as ALL_CAPS

    engine_openssl_missing = dict(
        text="<b>Shockingly, your system does not appear to have OpenSSL.</b>",
        type='info',
        icon='warning',
        timeout=default_timeout
        ),

I'd create a Message class that takes text, type, icon, and timeout as parameters. That gives some additional flexibility that might be helpful. It'll also make it easier to detect errors like misspelling one of the keys. I'd also go for a global constant for the types and icons like INFO = 0, WARNING = 2, for pretty much the same reason.

def infobar(self, id, filename=None, customtext=None, vbox=None):
    """Popup a new auto-hiding InfoBar."""


    if   msgs[id]['type'] in 'info':      msgtype = gtk.MESSAGE_INFO
    elif msgs[id]['type'] in 'question':  msgtype = gtk.MESSAGE_QUESTION
    elif msgs[id]['type'] in 'warning':   msgtype = gtk.MESSAGE_WARNING
    elif msgs[id]['type'] in 'error':     msgtype = gtk.MESSAGE_ERROR

You really want to use == not in. Actually the whole thing would be better as a fetch from a dictionary:

 msgtype = MSGTYPES[ msgs[id]['type'] ]

I'd also start with message = msgs[id] so as to avoid having msgs[id] over and over again.

    if   msgs[id]['icon'] in 'info':      imgtype = gtk.STOCK_DIALOG_INFO
    elif msgs[id]['icon'] in 'success':   imgtype = gtk.STOCK_APPLY
    elif msgs[id]['icon'] in 'question':  imgtype = gtk.STOCK_DIALOG_QUESTION
    elif msgs[id]['icon'] in 'warning':   imgtype = gtk.STOCK_DIALOG_WARNING
    elif msgs[id]['icon'] in 'error':     imgtype = gtk.STOCK_DIALOG_ERROR

    message = msgs[id]['text'].format(filename=filename, customtext=customtext)

    ibar                    = gtk.InfoBar()
    ibar.set_message_type   (msgtype)

I've not seen this style of spacing in python before. I dislike it, but its a personal style issue.

    if vbox:

if vox is not None is recommended instead.

        # If specific vbox requested: assume ibar for filemode, add cancel button
        ibar.add_button     (gtk.STOCK_CANCEL, gtk.RESPONSE_CANCEL)
        ibar.connect        ('response', self.cleanup_filemode)
    else:
        # If no specific vbox requested: do normal ibar at the top of message area
        vbox = self.vbox_ibar
        ibar.add_button     (gtk.STOCK_OK, gtk.RESPONSE_OK)
        ibar.connect        ('response', lambda *args: ibar.destroy())
    vbox.pack_end           (ibar, False, False)
    content                 = ibar.get_content_area()
    img                     = gtk.Image()
    img.set_from_stock      (imgtype, gtk.ICON_SIZE_LARGE_TOOLBAR)
    content.pack_start      (img, False, False)
    img.show                ()
    label                   = gtk.Label()
    label.set_markup        ("<span foreground='#2E2E2E'>{}</span>".format(message))
    content.pack_start      (label, False, False)
    label.show              ()
    # FIXME: Why doesn't Esc trigger this close signal?
    ibar.connect            ('close', lambda *args: ibar.destroy())
    ibar.show()
    if msgs[id]['timeout'] > 0:
        glib.timeout_add_seconds(msgs[id]['timeout'], ibar.destroy)
    return ibar

I'm not familiar with GUI toolkit you are using, so I can't comment much on this stuff

share|improve this answer
    
Ohhh I like it. At some point I had that idea -- to move all the messages into a single place, but scrapped it because of (1) inability to envision an elegant way to deal with the variable stuff like filenames and (2) the feeling that there were just too many messages. Hmmmm.... External file you say? XML too? Hmm. Innnteresting. Never would have thought of that. Already just went through a intro-tutorial on xml.etree.ElementTree (tho couldn't get it to import--weird), but before I reeally start trying to figure out implementation: why is xml the best solution for storage of this stuff? –  ryran Feb 28 '12 at 4:09
    
@ryan, I went for xml because the message content was already xml. –  Winston Ewert Feb 28 '12 at 4:24
    
Agh. So I've read through the ElementTree page in the docs, have gone through a few minimal tutorials, and have even gone through a few questions here and on stackoverflow (even one where you answered), but I'm having trouble doing the most basic thing--finding a message by id and returning the text. Can you point me to any resources Winston? –  ryran Feb 28 '12 at 14:19
    
@ryan, your best bet is probably to load all of the messages on startup and then store them in a dictionary by the id. To load a tag by the id directly from the xml, you'd need something like XPath, but thats probably not in your base python install. –  Winston Ewert Feb 28 '12 at 17:26
    
Hmm. Well I like the idea, but honestly this just leaves me back at square one -- I thought of this weeks ago but dismissed it because I couldn't see how to implement it. And I still don't. Oh well. In any case, thanks again for your time Winston. –  ryran Feb 28 '12 at 20:36
show 5 more comments

It's less readable. You made two surprising changes:

  • switching from a constant to an integer: this really impairs readability: you cannot know what it means without looking it up. You only save a few characters, not even a line or so.
  • switching from two parameters to a tuple: this makes little sense, since those are really two parameters. At least come up with a different name if you want to group them.

Did you consider implementing an infobar builder? If some common infobars are often built, you could create a wrapper to infobar, eg. infobar_info, which would accept the message and an enum saying which infobox you want to show. You can use "semantic names", eg. 'warn_user' will use a given timeout, message, icon and type that you always use to warn users, which will improve the consistency of the dialogs in your application.

share|improve this answer
    
I'm not familiar with what you mean by an infobar builder, but I think I get the idea. The problem is that there are so many various message, icon, type combinations. I'm slightly OCD with my coding so I've also tweaked all the timeouts--past a certain point, the timeouts are different depending on how long the message is. That said, I really appreciate your feedback and suspect a bunch of search & replace is in my future. –  ryran Feb 27 '12 at 20:30
    
I'm not seeing the switch from two parameters to a dictionary –  Winston Ewert Feb 28 '12 at 1:19
    
tuple, sorry. I prefer your answer anyway. –  Quentin Pradet Feb 28 '12 at 7:48
    
Bit of nitpick but since you've changed dictionary to tuple, the bit about reinventing named parameters doesn't make a lot of sense. –  Winston Ewert Feb 28 '12 at 21:30
    
@Cygal: Due to your comments and inspiration from Winston I've come up with a different way. I'd be curious to hear where you think it stands in terms of Pythonic style. –  ryran Feb 28 '12 at 22:30
show 3 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.