Code review comment for lp:~oliver-joos/zim/quick_fixes

On Tue, May 10, 2011 at 11:06 AM, Oliver Joos <email address hidden>wrote:

> No problem, I am glad about more eyes looking into this.
>
> 1/ Well, the positions are all stored in ONE SINGLETON dict that maps
> dialog class names to tuples (x, y). I don't see a problem with a global
> parameter here (testing? multiple zims running?). But I agree that it is not
> good OO-style. So I reimplemented it using a true Singleton class and added
> some helpful comments (see rev 396).
>

I'm sorry, but my problem was not the use of global as a construct to create
a singleton, but with the usage of a singleton in the first place. We use
singletons in a few places in the code, but only for things that are really
a shared resource (like the filesystem). In this case it is just an
interface setting, so the main ui object should store at as an instance
attribute. Not that using a singleton will immediately break the
application, but I just like to keep the design clean and consistent where
possible.

One option is to just add it in uistate but somehow allow flagging this
setting as something to be ignored when saving. Maybe just prefixing with an
"_" like "_windowpos" and modifying zim.config to ignore any attributes that
have a "_" prefix ?

Or maybe add a config dict in parallel to uistate which is not stored to a
config file for such settings that you want consistent over one instance of
zim only.

First option is most elegant to me, as it will allow you to have "private"
uistate parameters that are not carried over to the next instance, but they
go in the same place as all other ui state parameters, where it conceptually
belongs.

> 2/ My initial problem was a Zim page with some favicon examples. These .ico
> images are rendered correctly even in Linux, but are hidden by default in
> the InsertImageDialog. Zims rendering subsystem seems more capable than
> add_pixbuf_formats pretends! I did not find any reports on GTKs bugzilla (.
> gnome.org) about this. Anyway I fear it would take >1 year for a fix of
> add_pixbuf_formats to make it into distros like Ubuntu. So I simply added a
> fix into Zims wrapper function.
> The AttachFileDialog at the end of zim/__init__.py uses a hard-coded list
> (IMAGE_EXTENSIONS in fs.py) to decide if a file is an image. Instead of
> add_mime_type('image/*') I could add a filter for each entry in
> IMAGE_EXTENSIONS, or at least for '.ico', but I prefer my current solution.
> What do you think?
>

I would expect the add_pixbuf_types to map 1 to 1 with the rendering
backends that are available - this is the whole purpose of that routine, but
if this is not the case we can indeed just go for "image/*".

-- Jaap