# python pygtk code clarity vs conciseness

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!

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.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.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:
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.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.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:
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.

-
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

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.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.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:
return ibar


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

-
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

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.