hi Luka, On Tue, Dec 15, 2009 at 11:34:05PM +0100, Luka Čehovin wrote: > As I have said in my first email on this list my first self-assigned > task was to make mypaint store config according to XDG Base directory > specification. I have now committed a working version to my repository > (http://gitorious.org/~lukacu/mypaint/lukacu-mypaint).
Nice to see some work getting done :-) On Mon, Dec 14, 2009 at 09:30:24PM +0100, Luka Čehovin wrote: > SConstruct | 42 +++++++---- > gui/application.py | 33 ++++++--- > gui/backgroundwindow.py | 17 ++-- > gui/brushmanager.py | 30 ++++++-- > gui/drawwindow.py | 4 +- > gui/main.py | 8 +- > mypaint.c | 72 ++++++++++++++++++ > mypaint.py | 191 > ++++++++++++++++++++++++++++++++++++++--------- > 8 files changed, 311 insertions(+), 86 deletions(-) > create mode 100644 mypaint.c Woah, so much new code... do we really need all of this? Have you evaluated using pyxdg? Maybe it handles cross-platform? And I'm a bit surprised that you had to modify SConstruct. Those changes are not related to XDG support, mainly just for mypaint.c, right? > + for path in self.datapaths: > + themedir_src = join(path, 'desktop/icons') > + theme.prepend_search_path(themedir_src) Doesn't that reverse the order? Is this intentional, or does it not matter for some reason? > - self.pixmaps = PixbufDirectory(join(self.datapath, 'pixmaps')) > + self.pixmaps = PixbufDirectory(self.datapaths) Nice. > + def data_file(self, f): > + for dp in self.datapaths: > + if (os.path.exists(join(dp, f))): > + return join(dp, f) > + return None Seems a good idea to have such a method. Please don't put extra brackets around the condition in python. > class PixbufDirectory: > [...] > def __getattr__(self, name): > if name not in self.cache: > - pixbuf = gdk.pixbuf_new_from_file(join(self.dirname, name + > '.png')) > - self.cache[name] = pixbuf > + for tdir in self.dirnames: > + tdir = join(tdir, 'pixmaps') > + if os.path.exists(join(tdir, name + '.png')): Couldn't we call the data_file method instead? > +++ b/gui/backgroundwindow.py > [...] > - open(os.path.join(self.user_brushpath, 'order_default.conf'), > 'w').write(data) > + open(os.path.join(self.confpath, 'order_default.conf'), > 'w').write(data) Look like you moved order*.conf out of the brushes/ directory. Then you should also rename it to brush_order*.conf. Personally I think I like to have it in the brushes directory, so you can just copy this directory to get the same brush collection, and erase the remaining GUI configuration separately. But I guess it's fine either way. > --- /dev/null > +++ b/mypaint.c > [...] > + char launchscript[1024], searchpath[256]; > + token = strtok(lookup, ":"); > + while (token != NULL) Python string handling is so nice, why bother with C here? If it is only for a better process name in "top", then I think I'm against this. > (tested on my Ubuntu 9.10 machine). > Skeleton is ready to make it Windows compatible, but I would like to > have some feedback on that from windows developers. Bakhtiar / zakkum / tumagonx <[email protected]> (all the same person I think ;-) is the one to talk to here. He is sometimes on IRC. I think on Windows we basically know where things like GTK are installed and we can expect the users not to bother messing around with custom installation paths, so I wouldn't to put too much effort into doing something different there. So far it seems to have worked fine by simply doing the same thing as on Linux. And I would feel a bit bad about pushing a git version that is known to break on Windows. > def get_paths_linux(): > [... 73 lines of code ...] > + > +def get_paths_xp(): > + #TODO: implement > + raise Exception("Unsupported system") > + > +def get_paths_vista(): > + #TODO: implement > + raise Exception("Unsupported system") I don't think xp and vista (and 7) differ that much. And couldn't we maybe make the same code work for all systems? I don't like maintaining much special code for a platform that I can't test... > I would appreciate if somebody would take a few minutes and test it out. Disclaimer: I have not tested it yet. > - # note: some distros use lib64 instead, they have to edit this... > - lib_compiled='lib/mypaint/' > [...] > + for lpath in ['lib', 'lib32', 'lib64']: Also a looks like a hack, but looks better than the one before :-) > A few notes: > http://wiki.mypaint.info/index.php?title=User:Lukacu/Storage Maybe I should add that order.conf and the brush files go together, so order.conf might be a data file...? > * The old configuration directory is kept intact. mypaint now migrates > the data from it ... if it exists (it should also copy files if they > were modified in the old directory) If any files are copied to wrong > destination that is probably because I have made some wrong > assumptions. Hm... what about moving the files, but only if the new configuration directory does not exist yet? What use cases did you consider? I expect that a non-functional ~/.mypaint might also lead to confusion. If all solutions seem equally bad, I suggest we do the one that requires the fewest lines of code (eg. migrate nothing at all?) > * I have found out, that scons does not install icons correctly (on my > computer at least) and corrected this with some additional code. Strange, I thought we fixed that. Please review this to see if everything is consistent with your changes: http://gitorious.org/mypaint/mypaint/commit/cd4e89ff1e4a > (I have also noticed that this SConstruct is really Linux biased when it > comes to installing files (but have no plan of correcting this on my own)) Yes, I think it is used only for building on Windows. bye, Martin _______________________________________________ Mypaint-discuss mailing list [email protected] https://mail.gna.org/listinfo/mypaint-discuss
