Merge lp:~oliver-joos/zim/quick_fixes into lp:~jaap.karssenberg/zim/pyzim

[Oliver Joos]

Rev 394 of my quick_fixes branch fixes bug 587541 and bug 778542 of zim 0.52 (rev 391). So far I tested this branch sucessfully in Ubuntu 10.04.

[lp:~oliver-joos/zim/quick_fixes]

395. By Oliver Joos on 2011-05-06

Show also image filetypes like *.ico in InsertImageDialog

[Jaap Karssenberg (jaap.karssenberg)]

Afraid I can not merge it as-is

1/ the dialogs positions are stored in a global parameter, this is a major break of object-oriented programming, should stored as a object attribute in the interface object

2/ the filter for "image/*" needs explanation - in general this is used for dialogs selecting an image in the gtk interface, as the add_pixbuf_formats already adds all supported image formats all this will do is show images that are not supported in the dialog

review: Needs Fixing

[lp:~oliver-joos/zim/quick_fixes]

396. By Oliver Joos on 2011-05-10

Revised implementation to remember dialog positions

[Oliver Joos (oliver-joos)]

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

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?

[Jaap Karssenberg (jaap.karssenberg)]

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

[lp:~oliver-joos/zim/quick_fixes]

397. By Oliver Joos on 2011-05-10

Copy modification dates of images and attachments

398. By Oliver Joos on 2011-05-10

Even better solution to remember dialog positions

[Oliver Joos (oliver-joos)]

1/ Ah, now I got you right, and I like your idea of non-persistent uistate values by using names starting with '_'! I pushed another rev and indeed it fits much better than my previous attempts.
See http://bazaar.launchpad.net/~oliver-joos/zim/quick_fixes/revision/398

2/ Yes, I'd also expect add_pixbuf_types to match .ico, but in my uptodate Ubuntu 10.04, it does not.

[Oliver Joos (oliver-joos)]

@Jaap: Thanks for merging my branch!
I appreciate your small change (0,0)->(None,None) of my fix of bug 587541. And your fix copy->copy2 of bug 780184 is ok for me too. I did not dare to fix this in fs.py because this module is so central.

=== modified file 'zim/config.py'
--- zim/config.py	2011-04-14 17:41:49 +0000
+++ zim/config.py	2011-05-10 13:26:28 +0000
@@ -485,6 +485,9 @@
 	As a special case we can support sections that repeat under the
 	same section name. To do this assign the section name a list
 	before parsing.
+	
+	Values with keys starting with '_' are considered as non-persistent and are
+	only stored during one session of Zim. They will not appear in the INI file.
 	'''
 
 	def __getitem__(self, k):
@@ -547,12 +550,14 @@
 
 	def dump(self):
 		'''Returns a list of lines with text representation of the
-		dict. Used to write as a config file.
+		dict. Used to write as a config file. Values with keys starting with '_'
+		are considered as non-persistent and will be skipped.
 		'''
 		lines = []
 		def dump_section(name, parameters):
 			lines.append('[%s]\n' % section)
 			for param, value in parameters.items():
+				if not param.startswith('_'):
 				lines.append('%s=%s\n' % (param, self._encode_value(value)))
 			lines.append('\n')
 

=== modified file 'zim/gui/__init__.py'
--- zim/gui/__init__.py	2011-04-14 20:01:02 +0000
+++ zim/gui/__init__.py	2011-05-10 13:26:28 +0000
@@ -1308,6 +1308,7 @@
 				return None	# dialog was cancelled
 
 		file.copyto(dest)
+		os.utime(str(dest), (-1, file.mtime())) # copy modification date
 		return dest
 
 	def show_clean_notebook(self):
@@ -2084,7 +2085,10 @@
 		self.uistate = ui.uistate['MainWindow']
 
 		if not self._geometry_set:
-			# Ignore this is a explicit geometry was specified to the constructor
+			# Ignore this if an explicit geometry was specified to the constructor
+			self.uistate.setdefault('windowpos', (0, 0), check=value_is_coord)
+			x, y = self.uistate['windowpos']
+			self.move(x, y)
 			self.uistate.setdefault('windowsize', (600, 450), check=value_is_coord)
 			w, h = self.uistate['windowsize']
 			self.set_default_size(w, h)
@@ -2175,9 +2179,9 @@
 		#TODO: set toggle_readonly insensitive when page is readonly
 
 	def do_close_page(self, ui, page):
-		w, h = self.get_size()
 		if not self._fullscreen:
-			self.uistate['windowsize'] = (w, h)
+			self.uistate['windowpos'] = self.get_position()
+			self.uistate['windowsize'] = self.get_size()
 		self.uistate['sidepane_pos'] = self._zim_window_left_pane.get_position()
 
 	def do_textview_toggle_overwrite(self, view):

=== modified file 'zim/gui/searchdialog.py'
--- zim/gui/searchdialog.py	2011-02-19 16:27:44 +0000
+++ zim/gui/searchdialog.py	2011-05-10 13:26:28 +0000
@@ -116,5 +116,5 @@
 		# Popup find dialog with same query
 		if self.query and self.query.simple_match:
 			string = self.query.simple_match
-			string.strip('*') # support partial matches
+			string = string.strip('*') # support partial matches
 			self.ui.mainwindow.pageview.show_find(string, highlight=True)

=== modified file 'zim/gui/widgets.py'
--- zim/gui/widgets.py	2011-05-09 20:08:05 +0000
+++ zim/gui/widgets.py	2011-05-10 13:26:28 +0000
@@ -1890,6 +1890,11 @@
 		else:
 			self.uistate = zim.config.ListDict()
 
+		self.uistate.setdefault('_windowpos', (None, None), check=value_is_coord)
+		x, y = self.uistate['_windowpos']
+		if (x, y) != (None, None):
+			self.move(x, y)
+
 		self.uistate.setdefault('windowsize', defaultwindowsize, check=value_is_coord)
 		#~ print '>>', self.uistate
 		w, h = self.uistate['windowsize']
@@ -2023,6 +2028,8 @@
 			destroy = True
 
 		if ui_environment['platform'] != 'maemo':
+			x, y = self.get_position()
+			self.uistate['_windowpos'] = (x, y)
 			w, h = self.get_size()
 			self.uistate['windowsize'] = (w, h)
 			self.save_uistate()
@@ -2349,8 +2356,8 @@
 		return filter
 
 	def add_filter_images(self):
-		'''Wrapper for filechooser.add_filter()
-		using gtk.FileFilter.add_pixbuf_formats(). Returns the filter object.
+		'''Wrapper for filechooser.add_filter() to add a filter for images.
+		Returns the filter object.
 		'''
 		if len(self.filechooser.list_filters()) == 0:
 			self._add_filter_all()
@@ -2358,6 +2365,7 @@
 		filter.set_name(_('Images'))
 			# T: Filter in open file dialog, shows image files only
 		filter.add_pixbuf_formats()
+		filter.add_mime_type('image/*')       # to allow types like .ico
 		self.filechooser.add_filter(filter)
 		self.filechooser.set_filter(filter)
 		return filter