#73 Mouse cursor disappears if Preferences dialog is open

[Ark]

After opening the "Preferences" dialog, the mouse cursor is not visible in the main area anymore, even if you move it around. It is visible when hovering other areas such as the thumbnailer or the "Preferences" dialog, though. After closing the "Preferences" dialog, everything works as usual. Strangely enough, this does not happen with other dialogs such as the "Library" dialog or the "Enhance image" dialog.

[Ark]

I first thought that this has something to do with the _PreferencesDialog being modal (gtk.DIALOG_MODAL). However, the _EditArchiveDialog is modal as well but it does not have this issue.

Besides this, I wonder why the "Preferences" dialog is modal anyway, considering that changes made in the "Preferences" will be applied immediately.

[Ark]

After further investigation, it seems that the _EditArchiveDialog is actually affected as well. (Sometimes it looks as if everything's fine, though.) Also, removing the gtk.DIALOG_MODAL flag seems to fix this issue. Therefore, I would like to remove this flag from both dialogs.

However, the content of the _EditArchiveDialog should be consistent, i.e. the archive this window refers to should be the archive that is currently opened. Possible solutions that came to my mind:

• Close the dialog if the user opens another book.
• Close the dialog if the user opens a directory, otherwise update the content of the dialog.
• Always update the content of the dialog, show an appropriate message if the currently opened book is not an archive. In order to make things consistent, the "Edit archive" menu entry should always be enabled.
• Rename the corresponding menu entry to "Edit book" and allow editing of directories as well. This approach has the advantage that there would be no difference between archives and directories anymore. However, this might be more complicated to implement, considering that archive extraction is performed recursively while there is no recursion when using normal directories, for example.
• Make sure that the user cannot open another book or close the current one while the dialog is open. The problem here is that we might miss the closing event because of the window being closed for whatever (unknown or unexpected) reason.
• Keep the gtk.DIALOG_MODAL flag for the _EditArchiveDialog and enable/disable the "auto hide" feature by notifying the CursorHandler whenever we close/open a modal dialog. This approach has similar issues as the one just mentioned but to a lesser extent.

I think the last approach is the most reasonable one. I'm thinking of using a counter which starts at zero, is incremented whenever a modal dialog is opened, and is decremented whenever a modal dialog is closed. The "auto hide" feature is enabled iff the counter is equal to zero.

Does anybody know a better way to fix this or do I miss any important PyGTK features here?

[Benoit Pierre]

I remember looking into it: I tried to find a way to know whether the main window is able to receive events or not (because a modal dialog has focus). And gave up...

Another approach could be to disable/enable cursor auto hiding on dialog open/close, like this:

diff --git i/mcomix/dialog_handler.py w/mcomix/dialog_handler.py
index d08f3c7..ef8278e 100644
--- i/mcomix/dialog_handler.py
+++ w/mcomix/dialog_handler.py
@@ -15,18 +15,22 @@ dialog_windows[ 'properties-dialog' ] = [None, properties_dialog._PropertiesDial
 def open_dialog(action, window, name_of_dialog):
     """Create and display the given dialog."""


+    window.cursor_handler.auto_hide_off()
+
     _dialog = dialog_windows[ name_of_dialog ]

     # if the dialog window is not created then create the window
     # and connect the _close_dialog action to the dialog window
     if _dialog[0] is None:
     dialog_windows[ name_of_dialog ][0] = _dialog[1](window)

-    dialog_windows[ name_of_dialog ][0].connect('response', _close_dialog, name_of_dialog)
+    dialog_windows[ name_of_dialog ][0].connect('response', _close_dialog, window, name_of_dialog)
     else:
     # if the dialog window already exists bring it to the forefront of the screen
     _dialog[0].present()

-def _close_dialog(action, exit_response, name_of_dialog):
+def _close_dialog(action, exit_response, window, name_of_dialog):
+

+    window.cursor_handler.auto_hide_on()

     _dialog = dialog_windows[ name_of_dialog ]

@@ -35,5 +39,4 @@ def _close_dialog(action, exit_response, name_of_dialog):
     _dialog[0].destroy()
     _dialog[0] = None

-
 # vim: expandtab:sw=4:ts=4
diff --git i/mcomix/preferences_dialog.py w/mcomix/preferences_dialog.py
index d80d5a1..6e65bc6 100644
--- i/mcomix/preferences_dialog.py
+++ w/mcomix/preferences_dialog.py
@@ -465,7 +465,7 @@ class _PreferencesDialog(gtk.Dialog):

     def _response(self, dialog, response):
     if response == gtk.RESPONSE_CLOSE:

-        _close_dialog()
+        _close_dialog(self._window)

     elif response == constants.RESPONSE_REVERT_TO_DEFAULT:
         if self.notebook.get_nth_page(self.notebook.get_current_page()) == self.shortcuts:
@@ -482,7 +482,7 @@ class _PreferencesDialog(gtk.Dialog):

     else:
         # Other responses close the dialog, e.g. clicking the X icon on the dialog.

-        _close_dialog()
+        _close_dialog(self._window)

     def _create_language_control(self):
     """ Creates and returns the combobox for language selection. """
@@ -903,6 +903,8 @@ class _PreferencesDialog(gtk.Dialog):
 def open_dialog(action, window):
     """Create and display the preference dialog."""


+    window.cursor_handler.auto_hide_off()
+
     global _dialog

     # if the dialog window is not created then create the window
@@ -912,7 +914,9 @@ def open_dialog(action, window):
     # if the dialog window already exists bring it to the forefront of the screen
     _dialog.present()

-def _close_dialog():
+def _close_dialog(window):
+

+    window.cursor_handler.auto_hide_on()

     global _dialog

Maybe factorize the code a little too...

[Ark]

(Sorry, but I cannot apply the diff properly. svn patch runs without errors but the result does not. So I'm just guessing here.)

Thank you for the patch. I have thought of something similar, but as you have already mentioned, some refactoring might be a good idea. For example, the _edit_dialog.py file provides similar functions like open_dialog and _close_dialog, just as preferences_dialog.py does. Maybe they can be merged.

Furthermore, I still think that we need a counter here. I see at least two major flaws in the current approach without a counter:

• We must ensure that there is at most one open modal dialog at any given time.
• Each dialog needs to know about the cursor issue.

(By the way: There are at least two other modal dialogs, the "Library watch list" and the currently unnamed "Enter password to continue" window.)

Actually, I don't fully understand the purpose of the dialog_handler.py but it might be a good idea to merge the aforementioned open_dialog and _close_dialog functions into it. As a result, the remaining GUI code might not need to worry about this cursor issue (or similar issues we are not currently aware of). Instead, every time we want to open or close a window, we just call the functions there, like "open the Preferences window", and the dialog_handler.py knows exactly what to do (like disabling the autohide feature, or using the currently opened dialog instead of opening a new one, etc.).

What do you think?

By the way: Is there a reason why both BaseArchive and UnrarDll provide virtually identical implementations of _password_required?

[Benoit Pierre]

I can reproduce the bug with the edit archive dialog too.

Anyway, let's look at the list of all possible dialogs:

Name	Modal?
about	no
archive properties	no
edit archive	yes
edit bookmarks	no
edit commands	no
enhance image	no
library	no
library: new collection	yes
library: add	no
message	yes
open	no
preferences	yes
view comments	no

I think all dialogs linked to the current archive (archive properties, edit archive, enhance image, view comments) should be modal: to prevent an inconsistent state if switching to another archive/image while the dialog is open.

The message dialog should stay modal. I don't think there is a reason for the preferences dialog to be modal.

The bug does not happen when the dialog is not modal. So we only need to fix it for modal dialogs.

Two solutions:

• we deactivate cursor auto-hiding when a modal dialog is shown
• we keep cursor auto-hiding when the mouse hover on the main window, but fix the fact that it remains hidden

I think solution 2 makes the UI more consistent.

As for how it can be implemented, I found one way do it with very minimal changes:

diff --git i/mcomix/event.py w/mcomix/event.py
index d347093..7ae43de 100644
--- i/mcomix/event.py
+++ w/mcomix/event.py
@@ -577,9 +577,6 @@ class EventHandler:
                 self._last_pointer_pos_y = event.y_root
             self._drag_timer = event.time


-        else:
-            self._window.cursor_handler.refresh()
-
     def drag_n_drop_event(self, widget, context, x, y, selection, drag_id,
       eventtime):
         """Handle drag-n-drop events on the main layout area."""
diff --git i/mcomix/main.py w/mcomix/main.py
index 714ac07..c79b50a 100644
--- i/mcomix/main.py
+++ w/mcomix/main.py
@@ -272,6 +272,13 @@ class MainWindow(gtk.Window):
             self.actiongroup.get_action('library').activate()

         self.cursor_handler.auto_hide_on()

+        # Make sure we receive *all* mouse motion events,
+        # even if a modal dialog is being shown.
+        def _on_event(event):
+            if gtk.gdk.MOTION_NOTIFY == event.type:
+                self.cursor_handler.refresh()
+            gtk.main_do_event(event)
+        gtk.gdk.event_handler_set(_on_event)

     def gained_focus(self, *args):
         self.is_in_focus = True

What do you think?

Regarding BaseArchive/UnrarDll and the _password_required callback: UnrarDll used to not derive from the BaseArchive class, hence the duplication. I pushed a commit to get rid of it.

[Ark]

Awesome! This patch is really awesome! It solves the whole thing in a simple and highly sophisticated way. Well done! Please commit it. Thanks for the removal of the redundant code as well.

Concerning the modal issue in general: Thank you for the table and especially for the elaboration as it helps me thinking about this issue.

You are right when you say that the GUI should be consistent, and thus, for example, the "Enhance image" dialog should be modal. However, I think that there is an even better way, at least from a user's perspective. The issue here is: What if you want to check your "Enhance image" settings with several images? If the dialog were modal, you would be forced to close and re-open it every single time you want to switch to another image. This would be annoying.

Therefore, I think we should use four groups:

• Dialogs with no dependencies to the currently opened book ("none"). These dialogs should be non-modal and don't need to be notified about changes.
• Dialogs which only read from the currently opened book ("read book") or page ("read page"). These dialogs are non-modal as well but need to be notified whenever another book has been opened or the user has switched to another page, respectively.
• Dialogs which might trigger writing actions ("write"). These dialogs must be modal to prevent any of the aforementioned changes. Otherwise, the user might accidentally overwrite or leak data.

The following table shows how I think the dialogs should be classified:

Name	Dependencies
about	none
archive properties	read book
image properties	read page
edit archive	write
edit bookmarks	read page*
edit commands	read page
enhance image	read page
library	none
library: new collection	none
library: add	none
message	write(?)
open	none
preferences	none
save as	write
view comments	write(?)

Note that I have added a "save as" row in this table which is not present in the one you posted above. Also, I think that "message" covers dialogs such as "Delete image" or "Enter password" so I classified it as "write". (Please correct me if I am wrong here.) I think I have never seen the "view comments" dialog in action so "write" is merely a guess.

The "edit bookmarks" entry might be classified as "none", at least for the current version. However, I think it would be more fitting to add two buttons to this dialog instead: "Go to bookmark", which takes the user to the book and page identified by the currently selected bookmark, and "Add current page", which does the same as "Bookmark" → "Add Bookmark". The latter button would effectively turn this dialog into a "read page" dependent.

What do you think?

[Ark]

FYI: The "Go to bookmark" action mentioned above is actually the same as double-clicking a bookmark in the dialog.

[Benoit Pierre]

I pushed the fix.

Regarding the dialogs: what is important is whether access to the current book/page is needed during a dialog execution (and not just during construction). Which would make it modal.

• edit archive modifies the internal (in memory) list of pages for the current archive.
• view comments is read-only.
• the library dialogs are working fine, so I think we can ignore them for now.
• message dialogs are special, and should remain modal.

So to recap:

Name	Access (1)	Access (2)	Modal?
about			no
properties	book/page	(3)	no
edit archive	book	book	yes
edit bookmarks	book	book	no
edit commands		(4)	no
enhance image	page	(3)	no
go to page	page	book	yes
message	N/A	N/A	yes
open			no
preferences			no
save as	page	page	yes
view comments	book	(3)	no

(1): initial access
(2): access after construction
(3): those could be improved to update on book/page change
(4): the edit commands dialog 'preview' field already gets updated automatically on book/page change

So the first step would be to switch "preferences" to non-modal.

And then look at updating the dialogs marked with (3).

[Ark]

Thank you for the table and for the small code cleanup. Are you currently working on these two steps?

[Benoit Pierre]

Yes, but it might take a while... I'm wondering about cleaning up the file/image handler first:

• there are a bunch of corner cases if an archive as 0 images (so 0 pages, current page number None...)
• too much inbreeding between main/file_handler/image_handler

Anyway, you can see a WIP cleanup (with a properties dialog that handle book/page changes) here: https://github.com/benoit-pierre/mcomix/tree/dialogs-rework

[Ark]

Thank you very much for implementing this.

Actually, this update issue reminds me of what I called a Reader in a ticket I created a long time ago. See "Basic actions" in this post: https://sourceforge.net/p/mcomix/bugs/63/#c311 (You might want to read the whole ticket but it's both a bit long and dated.) Note that some of the things described there have been implemented already. For example, what I called a Scroll back then is now implemented in scrolling.py as Scrolling.

I attached a draft of a Reader. However, it only contains stubs at most and uses the abstraction of a Book which does not yet exist so it is not very useful right now. However, it might illustrate the overall purpose of a Reader.

It's not like I am going to change everything right now only to incorporate this Reader thing. I only want to inform you that I already thought of something like this so maybe you find some interesting ideas here and there.

[Benoit Pierre]

By the way, do you subscribe to all events for the project? Have you seen this [patches:#40] I posted?

Also, maybe a cleanup of the tracker is in order? I see a number of tickets that could be closed (I don't have rights to change a issue status, including my owns...):

• this ticket can be closed ;)
• [bugs:#62]: has been fixed in SVN for a while => closed.
• [bugs:#53]: not feasible => wont fix.
• [bugs:#49]: can't reproduce => closed.
• [bugs:#35]: using sftp://localhost in Thunar, and then opening in MComix works fine + the bug is really old => closed. Also direct GIO support is not possible since we may use some external executables that don't support it.
• [feature-requests:#5]: see above => wont fix.
• [feature-requests:#83]: wont fix?
• [feature-requests:#56]: wont fix?
• [feature-requests:#48]: wont fix?
• ...

And I'll stop there for now.

Personal rant:

Sourceforge sucks, there I said it... It's slow, the interface is clunky and buggy... I would be so happy if development was moved to Github.

[Benoit Pierre]

Ah, I see that you're currently doing just that: cleaning up the tracker :)

Regarding https://sourceforge.net/p/mcomix/bugs/63/#c311: I had already seen it. Unfortunately, both a rewrite, and cleaning up the current code is a lot of work... A progressive clean-up might be longer in the end, but may be the only practical approach.

[Ark]

Thank you for the ticket list. I was cleaning up there right when you posted this. However, I didn't fully understand all of them or couldn't decide what to do with them so I didn't close all of the ones you listed.

Yes, I think I subscribed to all relevant events, I also get a notification whenever you push any changes, and yes, I'm aware of [patches:#40]. Actually, I already sent oddegamra a message the other day but he didn't reply yet.

By the way, thank you very much for coding while I am "only" managing and testing things here. I hope it's okay for you. About your rant: I think I know what you mean and I am sorry this but there are various reasons why this isn't going to be changed any time soon.

Concerning [#63]: You are right, a progressive clean-up seems to be the only practical approach.

By the way, I am aiming for a bug fix release right now and I want this to be released in a few days or so. The reason for this are the more or less severe bugs we have found and fixed since 1.01, and at least from my perspective as a user, crashes and similar issues just spoil the whole experience. However, haste makes waste so don't change your pace. Instead, we should just try to keep things stable so we can release whenever we want, so to speak. (In other words, thank you for keeping the changes you're writing right now out of the SVN repository here. I know that this wouldn't be that much of an issue with a Git repository. I'm sorry.)

[Benoit Pierre]

If you want to do a bug fix release, I found another issue with the edit archive dialog, and I have a patch that fix the problem:

• edit dialog: fix exception when applying changes

I did not commit it yet because, well, the thumbnail code is pretty hairy as you know, but it seems to be working fine. Could you maybe test on your end?

Also 2 minor changes I'd like to push, regarding text contrast in some dialogs (I use a white on black GTK theme):

• properties dialog: fix main text contrast
• library: fix control area text contrast

On the subject of a new release, do you have a Windows machine to generate a new binary version? I tried using "setup.py dist_py2exe" with Wine while testing [patches:#40] — I wanted to see if czipfile was correctly included — but the resulting directory was missing all the DLLs.

[Ark]

Thank you for the patches, they are all bug fixes from my point of view. About the two "minor changes": Could you please add a constant for this #ddb color in constants.py (similar to constants.GTK_GDK_COLOR_BLACK) and use the constants there instead? Something like this:

:::python
GTK_GDK_COLOR_BOX_BACKGROUND = gtk.gdk.colormap_get_system().alloc_color(
                               gtk.gdk.color_parse('#ddb'), False, True)
GTK_GDK_COLOR_BOX_FOREGROUND = GTK_GDK_COLOR_BLACK

This might prevent cluttering the code.

[Benoit Pierre]

I went with another method: change the borderbox to use a gtk.Frame. And set the box state to gtk.STATE_ACTIVE. This way we get a box that uses valid theme colors, does not clash, has usable text contrast, while still being distinctive.

[Ark]

I just reviewed your changes. Looks good. Using gtk.STATE_ACTIVE and gtk.SHADOW_ETCHED_IN is a brilliant idea.

A minor thing that I just have noticed: You get an exception if you open the Properties dialog within an empty archive. Could you please disable the menu entry in this case? (It already gets disabled if you do not open anything, not even an empty archive.)

About releasing for Windows: oddegamra might lend a helping hand here, at least that's what he told me once.

[Ark]

Okay, it turns out that virtually everything you can interact with is prone to exceptions if you have opened an empty archive. I'm going to create a new ticket for this issue.

[Benoit Pierre]

Yeah, that's what I mentioned a few comments above: an archive with no images is trouble...

[Ark]

I see. Sorry, it had escaped my notice, I think.

Are you currently working on the "modal" issue? I think it's okay if we just fix the gtk.DIALOG_MODAL usages here even if we leave some dialogs "unreactive" (that is, they do not update their contents properly). This way, we wouldn't fix as many bugs as we have discussed here but I doubt that it is possible to do so in a few days anyway.

[Benoit Pierre]

Not actively, no.

I don't understand what you mean by:

I think it's okay if we just fix the gtk.DIALOG_MODAL usages here

The only easy change I can think of — but still maybe too dangerous if you want to do a bug fix release soon — is to switch the preferences dialog to non-modal. I just tested it, and it seems to work okay. Other modal dialogs should remain so. And then we can progressively update the non-modal dialogs to update on book/page change.

[Ark]

Sorry, the sentence wasn't clear. I just wanted to say that, maybe, we can just turn gtk.DIALOG_MODAL on or off for all the respective windows (according to the table above), ignoring that some of the then non-modal dialogs might need an appropriate update mechanism. This way, the "modal" property of the windows would be correct but the content of the windows might be out of date.

However, as you have already pointed out, this might be too dangerous.

A non-modal Preferences dialog should be okay, I think that, too. I just pushed a fix for that.

[Benoit Pierre]

OK, so I have updated my Github branch. The interesting changes are:

• improve support for empty archives:
  • image handler: fix page_is_available
  • image handler: fix get_path_to_page
  • main: fix _update_page_information
  • main: factorize _update_page_information a little
  • file handler: improve support for empty books
• rework page change and some preparatory changes for the dialogs rework
  • main: fix displayed_double
  • image handler: tweak get_virtual_double_page
  • file handler: small cleanup
  • file handler: cosmetic; fix indentation
  • rework page change code
  • openwith dialog: update to use page_changed
  • file handler: add file_closed callback
  • openwith dialog/menu: update to use file_closed
• rework non-modal dialogs: handle book/page change, support empty archives, don't block waiting for files...
  • properties dialog: rework to handle book/page change
  • enhance dialog: rework to handle book/page change
  • comment dialog: rework to handle book/page change

I would appreciate some feedback/testing.

[Ark]

Something came up so I didn't (and still don't) have that much time for MComix right now. Sorry to keep you waiting.

Thank you very much for your work. Here you are, some comments and suggestions on the commits. Please overlook that I might sound a bit brusquely here and there, I just need to be brief.

a059a193b69de16a2930a31acd59601cf7bb74ad

• Not yet tested.
• Some notes in [wiki:Maintenance] might be helpful here since there are some dependencies between source files and the ones introduced here, for example version numbers, package descriptions, links, etc.

ee678717a0942f16d089e4a0c222eb4a6c5d7b12

• Nice.

cafe4e8f20d95ae7c719f418be0b73e6cb168ebf

• Not yet tested.
• Could you please add a switch for the command line to override the try-except-approach and always import the "original" zipfile instead? Something like mcomix --force-import=zipfile would be nice. Same goes for subprocess32, e.g. mcomix --force-import=zipfile,subprocess. This way, users can fall back on the "old" implementations without changing the Python environment.

dede6956ddd210b010a03f4954a591a268b32ad0

• Not yet tested.

318af22f35e9a7941cc2582f8ad9671388fe1b4d (and other commits)

• Very good, fixes tons of issues. However, I just noticed that using the "Edit commands" dialog still raises exceptions if you have opened an empty archive and, for example, select the lines in the list one by one.

453a3c309be59c9168019d63fd8737bad21f73bd

• Helps a bit. (The major issue here (and in many other places in the code) remains: Using if-else code for 1 or 2 pages instead of a more generalized approach for n pages everywhere, but that's another issue.)

03bf6a90f43e7071a03d647ed1128e6773e5974a

• Could you please elaborate a bit more on this one?

ab7b4e318f5aee714d9fb689059c56e3e855703e

• I get an AssertionError when I try to go forward/back 10 pages but there are not enough pages to do so.
• Not "perfect" as it makes main.py even more powerful but seems to simplify the code.
• Note to self: Might need some more testing.

a222c8f0bd22faec5fb57f2a773aa26c36f852ef (and other commits)

• The refactored code looks nice.

212c3c4bfebb46345ceec5a7f184db294b2dcbca

• Good job!

All in all, I like it. Thanks again.