Menu ▾ ▴

#73 Mouse cursor disappears if Preferences dialog is open

SVN
closed-fixed
nobody
None
5
2015-09-17
2015-02-06
Ark
No

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.

Related

Bugs: #75
Bugs: #76

Discussion

<< < 1 2 3 > >> (Page 2 of 3)

  • Benoit Pierre

    Benoit Pierre - 2015-02-25

    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.

    No problem. :)

    archlinux: add directory

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

    I don't really intend on pushing this commit to Subversion. It's just something I use to conveniently handle modified packages on Arch Linux.

    fix mcomixstarter.py python version (github.com)

    • Nice.

    Another custom modification for the sake of convenience; this one has been bothering me for a while :p. I can push this, together with updated:

    • mcomixstarter.py
    • mime/comicthumb
    • setup.py
    • install instructions in README

    archive: use czipfile for ZIP support if available

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

    Why? Why would you want to switch to the old implementation? There is really no benefit, and IMHO not much risks in using czipfile instead. As stated on the project page: the code is actually 95% identical to Python 2.6.5’s Lib/zipfile.

    file handler: tweak "continue reading from..." dialog (github.com)

    • Not yet tested.

    What about the behaviour change? Are you okay with it?

    empty archives support

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

    Fixed.

    main: factorize _update_page_information a little

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

    image handler: tweak get_virtual_double_page

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

    As mentioned in the commit message, the goal is to support calling get_virtual_double_page for checking another page than the current one (needed by the next batch of commits).

    rework page change code

    • I get an AssertionError when I try to go forward/back 10 pages but there are not enough pages to do so.

    Fixed.

    • Not "perfect" as it makes main.py even more powerful but seems to simplify the code.

    It's better than having the logic for page change all over the place (main + image handler + events). The goal is also to cut down on module inter dependencies, especially circular dependencies...

    • Note to self: Might need some more testing.

    use page_changed/file_closed callbacks

    • The refactored code looks nice.

    properties dialog: rework to handle book/page change

    • Good job!

    Do you have time to make a bug fix release? I think releasing the current state before pushing anything more is a good idea. No?

    I also have quite a few more changes (testsuite for archives (with a few bug fixes), thumbbar handling, process handling simplification, ...) and a few other things (fate of mimes/comicthumb, PIL use on Windows) I'd like to discuss when you have time.

     

  • Ark

    Ark - 2015-03-01

    Are you sure that you've pushed your changes? origin/bpierre still points to 93eea80c9e6b3a5320cef88b5e32c248970a99e9 (2015-02-20).

     

  • Benoit Pierre

    Benoit Pierre - 2015-03-01

    I had not. I just pushed my current state.

     

  • Ark

    Ark - 2015-03-07

    I'm back. Sorry to keep you waiting. I will reply to your post from 2015-02-25.

    I don't really intend on pushing this commit to Subversion. It's just something I use to conveniently handle modified packages on Arch Linux.

    I see.

    fix mcomixstarter.py python version (github.com)

    • Nice.

    Another custom modification for the sake of convenience; this one has been bothering me for a while :p. I can push this, together with updated:
    - mcomixstarter.py
    - mime/comicthumb
    - setup.py
    - install instructions in README

    I guess I will see those changes in newer commits.

    archive: use czipfile for ZIP support if available

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

    Why? Why would you want to switch to the old implementation? There is really no benefit, and IMHO not much risks in using czipfile instead. As stated on the project page: the code is actually 95% identical to Python 2.6.5’s Lib/zipfile.

    I understand why you want to see czipfile in action. I have in mind that the switch should work as a fallback option if something breaks so that the user does not need to wait for an update that fixes it. (A possible workaround for the user would be extracting the archive(s) manually.) On the other hand, I cannot recall cases of users complaining about slow extraction of encrypted zip files, so it seems to be a very rare case anyway. Therefore, I think now that the switch I proposed is actually not that important. Just keep the current czipfile solution the way it is now.

    file handler: tweak "continue reading from..." dialog (github.com)

    • Not yet tested.

    What about the behaviour change? Are you okay with it?

    I just tested it and yes, the new behavior feels more naturally to me.

    empty archives support

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

    Fixed.

    Thanks. :)

    image handler: tweak get_virtual_double_page

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

    As mentioned in the commit message, the goal is to support calling get_virtual_double_page for checking another page than the current one (needed by the next batch of commits).

    I see, thank you for the explanation.

    rework page change code

    • I get an AssertionError when I try to go forward/back 10 pages but there are not enough pages to do so.

    Fixed.

    Thanks. :)

    • Not "perfect" as it makes main.py even more powerful but seems to simplify the code.

    It's better than having the logic for page change all over the place (main + image handler + events). The goal is also to cut down on module inter dependencies, especially circular dependencies...

    Yes, indeed.

    Do you have time to make a bug fix release? I think releasing the current state before pushing anything more is a good idea. No?

    I also have quite a few more changes (testsuite for archives (with a few bug fixes), thumbbar handling, process handling simplification, ...) and a few other things (fate of mimes/comicthumb, PIL use on Windows) I'd like to discuss when you have time.

    I have some more time now, possibly enough for a new release. (I hope so!) However, I am a bit confused about what you mean by "current state". The commits I reviewed so far do not exist anymore, at least that is what the hash codes tell me. Also, there are newer commits in your repository. Could you please tell me which one of the commits I can currently see in your repository is the "current state"?

    I guess that 841d51f77dd34f218d493b1964e94db96d6b104f corresponds to the 93eea80c9e6b3a5320cef88b5e32c248970a99e9 commit I mentioned earlier (because the timestamps match). That means that I haven't seen any of the commits newer than this one. I would like to review the commits from this one up to this "current state" commit.

     

  • Ark

    Ark - 2015-03-08

    Thank you for clarifying. A rundown would be nice. I'm currently testing [r1125] a bit. I just found some inconsistencies when you try to use Ctrl+PgUp or Ctrl+PgDown to go to another book. Please have a look at the following table.

    Test case Can open explicitly Can navigate to Can navigate from
    Empty archive Yes Yes Yes
    Empty directory Yes No No
    Archive without reading permission Yes No No
    Directory without reading permission Yes No No

    I think it would be better this way:

    Test case Can open explicitly Can navigate to Can navigate from
    Empty archive Yes Yes Yes
    Empty directory Yes Yes Yes
    Archive without reading permission Yes No Yes
    Directory without reading permission Yes No Yes

    Also, when an empty archive is opened, the "Edit commands" dialog raises exceptions (because it cannot expand symbols like %f in the current context, I guess). This does not happen in any of the other cases if I tested it correctly.

     

    Related

    Commit: [r1125]


    Last edit: Ark 2015-03-14

  • Benoit Pierre

    Benoit Pierre - 2015-03-08

    Empty directories handling

    So if I have the following filesystem tree:

    /cwd
/cwd/tmp1/image.png
/cwd/tmp2/image.png
/cwd/tmp3
/cwd/tmp4/image.png

    And I start MComix like this: mcomix tmp1, then 'Ctrl+PgDown' will show tmp2, and pressing it again will show tmp4, skipping over the empty tmp3 directory, which I think make sense, no? You would like tmp3 to be shown, even if empty?

    Exceptions in "Edit commands" dialog when archive is empty

    This is fixed in my branch.

     

  • Ark

    Ark - 2015-03-08

    And I start MComix like this: mcomix tmp1, then 'Ctrl+PgDown' will show tmp2, and pressing it again will show tmp4, skipping over the empty tmp3 directory, which I think make sense, no? You would like tmp3 to be shown, even if empty?

    Yes, this is correct, I would like to see an empty tmp3 in this example. Reasoning:

    • Expectations. Imagine you get a bunch of image directories you have to review. However, one of the directories is full of images MComix is unable to read (unknown format). If this directory is being skipped, you might accidentally forget to review the images in this directory, thinking that you have seen all the images you wanted to see.
    • Runtime performance issues. If you hit Ctrl+PgDown but the following 100 directories or so contain about 1000 files each, none of them being an image, MComix will take a long time scanning directories for nothing.

    Wrong permissions are not that much of an issue, though: Checking permissions is done fast and if you are in a directory with tons of files and/or directories you are not permitted to read, you are most probably in the wrong directory anyway.

    If you are interested in it, we can introduce a pref for this. However, this would be an issue for another release.

    This is fixed in my branch.

    I think we need this fix for a release.

     

  • Benoit Pierre

    Benoit Pierre - 2015-03-08

    This is fixed in my branch.

    I think we need this fix for a release.

    This patch does not depend on others, so it can be pushed to Subversion, but the "empty archive" support would still not be complete: like with the properties dialog.

    It would make more sense then to push the whole batch.

     

  • Ark

    Ark - 2015-03-10

    And here is a rundown of the new changes in my branch:

    Thank you very much, it is very helpful. I hope it's okay for you if I skip commenting on some of the commits since I don't have always something profound to say.

    archlinux

    Custom Arch Linux support, you can ignore this one.

    comicthumb: fix Image import (update to PIL)

    Self-explanatory.

    comicthumb: rewrite to use mcomix

    Note: it's slower, because of some of the import/code at initialization
    (GTK/GDK imports, looking for libunrar.so, ...), but I think it's better to
    avoid the duplication of code, and support the same formats (archives and
    images) than MComix itself.

    I have to admit that I don't know much about these (seemingly high-level) things. What is the purpose of comicthumb? For me, it looks like some file browser like Nautilus might want to run this code to create and display thumbnails. Is this correct?

    What happens if this code runs into an encrypted archive? (Just as I said, I don't know when this code is supposed to be executed so I can't test it myself.)

    Any way, I both like and dislike this change at the same time. That is, I totally agree with what you are trying to accomplish but I think it's not a good idea to do it this way. For example, thumbnail_tools.Thumbnailer() depends on prefs so if the preferences are broken for some reason, it also breaks comicthumb even if it does not actually need prefs.

    Therefore, I think it's better to do it this way: Try to refactor source files like thumbnail_tools.py to isolate the shared code. That is, comicthumb should be able to call the functions it needs directly, without the need to load any libraries, prefs, etc. it does not depend on. Do you know how Python behaves on imports? If an import makes Python run code, it might be a good idea to move the functions shared by comicthumb to separate code files which only import the packages the shared functions really need.

    What do you think? Any other suggestions or objections?

    archive: fix non-lazy logging calls
    file handler: minor cleanup, remove duplicate call

    Okay.

    file handler: small code simplification

    Sorry, I did not yet understand the new code.

    remove a bunch of unused imports

    Wow, nice one. (I hope this doesn't break anything.)

    password dialog: show archive path

    It can be useful to know the name of the archive whose password is being asked for.

    Good idea. Small fix: I think it's better to use %s instead of concatenating the archive's path here. Just copy the way it is done in other parts of the code, for example (extracted from some source file):

    :::python
msg = _("No images in '%s'") % os.path.basename(self._current_file)

    constants: fix GTK import

    Already included later in the file, and with a catch for a possible ImportError so mcomix/run.py can do its thing (detect and warn if GTK cannot be imported).

    I see. Well spotted!

    drop unused mcomix/debug

    No need for this anymore I think. Better to use the logger.

    Okay.

    run: add SIGCHLD handler to reap zombie processes

    Don't leave zombies when calling external commands.

    This issue bothered me for a long time. Thank you very much for fixing it.

    process simplification

    Looking at the use cases, there are 2:
    - calling a command, we don't care about its output
    - calling a command, we need its output

    So simplify the code to cover those. Also, simplify the use of external archive extractors: redirect stdout to output file, instead of reading it and then writing it ourselves.

    process: add find_executable
    archive: simplify checks for external executables

    Use distutils.spawn.find_executable: no point in spawning a subprocess.

    Interesting changes. Maybe you can declutter the code a bit even further by making find_executable accept iterables of strings so the code here …

    :::python
        global _rar_executable
        if _rar_executable != -1:
            return _rar_executable
        for exe in (u'unrar', u'rar'):
            _rar_executable = process.find_executable(exe)
            if _rar_executable:
                break
        return _rar_executable

    … can be changed to look like this:

    :::python
        global _rar_executable
        if _rar_executable == -1:
            _rar_executable = process.find_executable((u'unrar', u'rar'))
        return _rar_executable

    Note that in the example above, I changed the idiom to remove one of the return statements. The source files for the other archive types may be simplified this way as well.

    Does from distutils import spawn also introduce a new external dependency?

    test/archives
    archive: fix tar listings
    test/archives: add tests for option like entries
    archive: fix external RAR when an entry starts with @
    test/archives: add tests for glob like entries
    archive: fix external ZIP for glob like entries
    test/archives: mark some tests as xfail
    test/archives: add password tests
    archive: fix possible hang with external ZIP
    test/archives: mark some tests as xfail
    archive: cosmetic
    test/archives: add basic recursive archive tests
    archive: fix RecursiveArchive init
    test/archives: add basic unicode tests
    test: change __init__
    test/archives: improve handling of temporary directories
    test/archives: mark some tests as xfail
    test/archives: remove unused code

    Add a testsuite for mcomix/archive, and fix a bunch of small issues found thanks to it. Note: if this is to be pushed to Subversion, then I'll commit the archive samples in test/files/archives/ (it's pretty small: about 240K); since they can be automatically created on Linux, but not (always) on Windows.

    I think I got the idea why "option-like entries" need special treatment but I did not yet understand what actually happens if they occur. Furthermore, I am not used to testing in Python so I cannot say much about it here except that, apparently, it is a very good idea and the test suite seems to cover a lot of corner cases. Thank you very much.

    I think it is okay if you commit the archive examples as well. (By the way, it's not 240K but only 24K.)

    [wip] win-install.txt (github.com)

    Personal notes for setuping a Windows environment for testing. Could be added to a new HACKING readme or section in README?

    Maybe the Wiki is a better place for these kinds of notes. How about a new section in https://sourceforge.net/p/mcomix/wiki/Maintainance/?

    thumbbar: cleanup

    • use 'cursor-changed' signal instead of 'changed':
      avoid the need for the _selection_is_forced hack
      to detect selection on mouse click
    • fix GTK warning: IA__gtk_tree_view_scroll_to_cell:
      assertion 'tree_view->priv->tree != NULL' failed

    This one has bugged me as well. One thing I would like to see changed, though:

    :::python
# This line

uri = 'file://localhost' + urllib.pathname2url(path)

# ... should be replaced by this one:

uri = tools.path_to_url(path)

# For this to work, the following function must exist in tools.py.
# (I did not test it, though.)
# It originates from
# https://stackoverflow.com/questions/11687478/convert-a-filename-to-a-file-url#14298190
import urlparse, urllib
def path_to_url(path):
    return urlparse.urljoin('file:', urllib.pathname2url(path)) # XXX better use u'...'?

    thumbbar: align selection on change

    Attempt at fixing the fact that on startup, the selected tumbnail in the thumbbar is not in view... Aligning the selection so it stays 1/4 in the view is the only way I found to make it visible, but it does change the behaviour a little. Note: this is not done on mouse click, as it would be annoying. Also, the selected thumbnail does not appear as such on startup (background color not active), so still a work in progress...

    This bug is quite annoying. However, I think it is more important to have the current image highlighted than to see it in the thumbnailer. (That is, the old bug here is better than the new one.) It would be nice to see this one fixed as well, but if it turns out to be surprisingly difficult, it might be better to return to the old behavior and postpone the fix to another release.

    I suspect that this has something to do with the rearrangement of the items in the list if some of the thumbnails turn out to be of a different size than expected, but this is merely a wild guess.

    fix "don't act on click to focus" handling

    It was only working if the mouse pointer had passed over the main area; i.e. the following sequence would not work correctly:
    - focus out
    - move pointer hover the thumbbar, "entering" the window from the left
    - click on the thumbbar: => the page is changed

    Wow, very well spotted! Thank you.

    file handler: improve 'goto last read page?' popup

    Show the first page and start loading thumbnails before showing it:
    - it gives some context (first page = cover)
    - there's a 50% chance we selected the right page
    - if the archive is solid, it doesn't matter anyway

    Not sure about this one, since we don't always show the cover: when switching from the next archive with Ctrl+PageUp => last page is shown. So maybe better to start showing the last read page, what do you think?

    Good question. I think it's better to separate the way books are being opened from this dialog. That is, for example, if you press Ctrl+PageUp, MComix should always go to the last page first. Then, if MComix wants to ask the user, the dialog appears. However, if MComix has been told to "Do not ask again", MComix should go straight to the respective page (that is, the last read page or the first page, depending on what has been chosen beforehand).

    There is an inconsistency: If you "Go -> Next archive" to a book, you get asked whether to go to the last read page or not. However, if you "Go -> Previous archive" to the very same book, you do not get asked.

    list of supported image formats: there can only be one!

    This way we get support for all GdkPixbuf supported formats (like WebP if gdk-pixbuf-webp is available).

    Going forward, I think it would be best if the code used image_tools.is_image_file only, and constants.SUPPORTED_IMAGE_REGEX was folded into image_tools: this make it easier to change the library used for loading images (like using PIL, see below).

    What do you mean by "see below"? I think the solution you offer with this commit is good enough. So how are further modifications supposed to make things easier? Sorry, I just don't fully understand.

    thumbnail tools: small optimization
    thumbnail tools: fix possible PIL exception
    image handler: add debug trace on thumbnail error

    Okay.

    image tools: add get_image_info

    Nice. A function that extracts metadata might be very useful for better caching algorithms, for example.

    image handler: remove GDK dependency
    thumbnail tools: remove _get_text_data dependency on PIL

    Okay.

    [wip] thumbnail tools: _thumbnail_exists; PIL -> GDK

    Same rational as above: try to reduce image library dependency to image_tools. Also avoid inconsistencies: using PIL for image information when GdkPixbuf is actually used to load images.

    I understand what you are trying to accomplish but I did not yet fully understand the code.

    image tools: optimize load_pixbuf_size

    Right now, when creating a thumbnail, the code will load the full image, and then use the current scaling mode preferences to resize it. This can be really slow. I personally don't see the point in generating a 128x128 thumbnail with Hyperbolic scaling, when the result of gdk.pixbuf_new_from_file_at_size is good enough for this. Also, it's much faster for image formats like JPEG, or WebP: the decoder can be configured to automatically scale down the output of the decoding.

    I see. Maybe comicthumb can benefit from this, too. (Not sure, though.)

    image tools: disable force use of PIL on Windows

    I was looking at using PIL (initially for WebP support), and my tests shows that it is much slower than GdkPixbuf: both on Linux and on Windows (testing with Wine). So I wonder if this still make some sense? I don't see the "GTK slowness" mentioned in the comment, maybe because I'm using GTK version 2.24.2 instead of 2.18.2. Do you have a real Windows machine to test this?

    Sorry, I did not benchmark this and I don't have access to a Windows machine.

    use open() instead of file()
    osd: use mcomix.constants for black/white colors (github.com)
    do not use gtk.gdk.colormap_get_system (github.com)
    use gtk.main_iteration_do instead of gtk.main_iteration (github.com)
    status bar: do not use -1 for context id
    status bar: do not use widget.set_data/get_data (github.com)
    do not use deprecated dialog.set_has_separator (github.com)
    do not use pango.SCALE_XXX constants (github.com)

    I was looking at whether updating to GTK3 is possible, and actually have a version that starts (and somewhat works) after tweaking the code a little. Those changes are needed, and still compatible with GTK2, so I think they can be pushed, what do you think?

    Interesting. I don't know much about the differences between GTK2 and GTK3 (admittedly, I don't even know GTK2 very well) so I'm glad you're dealing with this issue. Actually, I don't like the idea that switching to another library might break something. On the other hand, these changes don't look like they are going to break things easily. Is it okay for you to postpone these "in prevision of GTK3 support" changes for a later release?

    Again, thank you very much for your work. It took me a while to review all these commits. Is it possible to push only some of them to SVN? If so, I would like to choose the ones to be included in the upcoming release.

     

    • Benoit Pierre

      Benoit Pierre - 2015-03-11

      comicthumb: rewrite to use mcomix

      Note: it's slower, because of some of the import/code at initialization
      (GTK/GDK imports, looking for libunrar.so, ...), but I think it's better to
      avoid the duplication of code, and support the same formats (archives and
      images) than MComix itself.

      I have to admit that I don't know much about these (seemingly high-level) things. What is the purpose of comicthumb? For me, it looks like some file browser like Nautilus might want to run this code to create and display thumbnails. Is this correct?

      Yes.

      What happens if this code runs into an encrypted archive? (Just as I said, I don't know when this code is supposed to be executed so I can't test it myself.)

      It won't work on encrypted archives.

      Any way, I both like and dislike this change at the same time. That is, I totally agree with what you are trying to accomplish but I think it's not a good idea to do it this way. For example, thumbnail_tools.Thumbnailer() depends on prefs so if the preferences are broken for some reason, it also breaks comicthumb even if it does not actually need prefs.

      Therefore, I think it's better to do it this way: Try to refactor source files like thumbnail_tools.py to isolate the shared code. That is, comicthumb should be able to call the functions it needs directly, without the need to load any libraries, prefs, etc. it does not depend on. Do you know how Python behaves on imports? If an import makes Python run code, it might be a good idea to move the functions shared by comicthumb to separate code files which only import the packages the shared functions really need.

      What do you think? Any other suggestions or objections?

      I agree that preferences should only be accessed by higher level (near the GUI) code. comicthumb is also really not a priority for me (I don't use a graphical file manager). I also wonder how many MComix users use it: nobody on Windows, and on Linux, there are probably other thumbnailers available. This way was the easiest and the fastest, and ensure we get a consistent behaviour between comicthumb and MComix: same supported formats (archive+images), same cover detection.

      password dialog: show archive path

      It can be useful to know the name of the archive whose password is being asked for.

      Good idea. Small fix: I think it's better to use %s instead of concatenating the archive's path here.

      Doing it this way means the gettext translations don't need to be updated. I also think it's better to show the full path.

      process: add find_executable
      archive: simplify checks for external executables

      Use distutils.spawn.find_executable: no point in spawning a subprocess.

      Interesting changes. Maybe you can declutter the code a bit even further by making find_executable accept iterables of strings [...]

      Agreed.

      Does from distutils import spawn also introduce a new external dependency?

      No.

      test/archives
      archive: fix tar listings
      test/archives: add tests for option like entries
      archive: fix external RAR when an entry starts with @
      test/archives: add tests for glob like entries
      archive: fix external ZIP for glob like entries
      test/archives: mark some tests as xfail
      test/archives: add password tests
      archive: fix possible hang with external ZIP
      test/archives: mark some tests as xfail
      archive: cosmetic
      test/archives: add basic recursive archive tests
      archive: fix RecursiveArchive init
      test/archives: add basic unicode tests
      test: change __init__
      test/archives: improve handling of temporary directories
      test/archives: mark some tests as xfail
      test/archives: remove unused code

      Add a testsuite for mcomix/archive, and fix a bunch of small issues found thanks to it. Note: if this is to be pushed to Subversion, then I'll commit the archive samples in test/files/archives/ (it's pretty small: about 240K); since they can be automatically created on Linux, but not (always) on Windows.

      I think I got the idea why "option-like entries" need special treatment but I did not yet understand what actually happens if they occur.

      We may not get the data we asked for (when using the external ZIP executable), because the entry will be interpreted as a pattern.

      Furthermore, I am not used to testing in Python so I cannot say much about it here except that, apparently, it is a very good idea and the test suite seems to cover a lot of corner cases. Thank you very much.

      I think it is okay if you commit the archive examples as well. (By the way, it's not 240K but only 24K.)

      No, the current directory contains only 24K, but the testsuite generate more test files:

      • test/files/archives/Flat.7z
      • test/files/archives/Flat.lha
      • test/files/archives/Flat.rar
      • test/files/archives/Flat.zip
      • test/files/archives/GlobEntries.7z
      • test/files/archives/GlobEntries.lha
      • test/files/archives/GlobEntries.rar
      • test/files/archives/GlobEntries.zip
      • test/files/archives/OptEntry.7z
      • test/files/archives/OptEntry.lha
      • test/files/archives/OptEntry.rar
      • test/files/archives/OptEntry.zip
      • test/files/archives/Password.7z
      • test/files/archives/Password.rar
      • test/files/archives/Password.zip
      • test/files/archives/SolidFlat.7z
      • test/files/archives/SolidFlat.rar
      • test/files/archives/SolidFlat.tar
      • test/files/archives/SolidFlat.tar.bz2
      • test/files/archives/SolidFlat.tar.gz
      • test/files/archives/SolidGlobEntries.7z
      • test/files/archives/SolidGlobEntries.rar
      • test/files/archives/SolidGlobEntries.tar
      • test/files/archives/SolidGlobEntries.tar.bz2
      • test/files/archives/SolidGlobEntries.tar.gz
      • test/files/archives/SolidOptEntry.7z
      • test/files/archives/SolidOptEntry.rar
      • test/files/archives/SolidOptEntry.tar
      • test/files/archives/SolidOptEntry.tar.bz2
      • test/files/archives/SolidOptEntry.tar.gz
      • test/files/archives/SolidPassword.7z
      • test/files/archives/SolidPassword.rar
      • test/files/archives/SolidTree.7z
      • test/files/archives/SolidTree.rar
      • test/files/archives/SolidTree.tar
      • test/files/archives/SolidTree.tar.bz2
      • test/files/archives/SolidTree.tar.gz
      • test/files/archives/SolidUnicode.7z
      • test/files/archives/SolidUnicode.rar
      • test/files/archives/SolidUnicode.tar
      • test/files/archives/SolidUnicode.tar.bz2
      • test/files/archives/SolidUnicode.tar.gz
      • test/files/archives/Tree.7z
      • test/files/archives/Tree.lha
      • test/files/archives/Tree.rar
      • test/files/archives/Tree.zip
      • test/files/archives/Unicode.7z
      • test/files/archives/Unicode.lha
      • test/files/archives/Unicode.rar
      • test/files/archives/Unicode.zip

      The total size of the directory with these new files is 240K.

      [wip] win-install.txt (github.com)

      Personal notes for setuping a Windows environment for testing. Could be added to a new HACKING readme or section in README?

      Maybe the Wiki is a better place for these kinds of notes. How about a new section in https://sourceforge.net/p/mcomix/wiki/Maintainance/?

      I'm not too fond of the Wiki, I prefer the documentation to be versioned alongside the code.

      thumbbar: cleanup

      • use 'cursor-changed' signal instead of 'changed':
        avoid the need for the _selection_is_forced hack
        to detect selection on mouse click
      • fix GTK warning: IA__gtk_tree_view_scroll_to_cell:
        assertion 'tree_view->priv->tree != NULL' failed

      This one has bugged me as well. One thing I would like to see changed, though:

      :::python
# This line

uri = 'file://localhost' + urllib.pathname2url(path)

# ... should be replaced by this one:

uri = tools.path_to_url(path)

# For this to work, the following function must exist in tools.py.
# (I did not test it, though.)
# It originates from
# https://stackoverflow.com/questions/11687478/convert-a-filename-to-a-file-url#14298190
import urlparse, urllib
def path_to_url(path):
    return urlparse.urljoin('file:', urllib.pathname2url(path)) # XXX better use u'...'?

      I really don't know about this part of the code. I don't use a DE, or a graphical file manager, so the drag&drop code is of no use to me. Does it not work with the actual code?

      thumbbar: align selection on change

      Attempt at fixing the fact that on startup, the selected tumbnail in the thumbbar is not in view... Aligning the selection so it stays 1/4 in the view is the only way I found to make it visible, but it does change the behaviour a little. Note: this is not done on mouse click, as it would be annoying. Also, the selected thumbnail does not appear as such on startup (background color not active), so still a work in progress...

      This bug is quite annoying. However, I think it is more important to have the current image highlighted than to see it in the thumbnailer. (That is, the old bug here is better than the new one.) It would be nice to see this one fixed as well, but if it turns out to be surprisingly difficult, it might be better to return to the old behavior and postpone the fix to another release.

      I suspect that this has something to do with the rearrangement of the items in the list if some of the thumbnails turn out to be of a different size than expected, but this is merely a wild guess.

      Maybe.

      file handler: improve 'goto last read page?' popup

      Show the first page and start loading thumbnails before showing it:
      - it gives some context (first page = cover)
      - there's a 50% chance we selected the right page
      - if the archive is solid, it doesn't matter anyway

      Not sure about this one, since we don't always show the cover: when switching from the next archive with Ctrl+PageUp => last page is shown. So maybe better to start showing the last read page, what do you think?

      Good question. I think it's better to separate the way books are being opened from this dialog. That is, for example, if you press Ctrl+PageUp, MComix should always go to the last page first. Then, if MComix wants to ask the user, the dialog appears. However, if MComix has been told to "Do not ask again", MComix should go straight to the respective page (that is, the last read page or the first page, depending on what has been chosen beforehand).

      There is an inconsistency: If you "Go -> Next archive" to a book, you get asked whether to go to the last read page or not. However, if you "Go -> Previous archive" to the very same book, you do not get asked.

      I think it's by design. I personally don't like the behaviour when switching page/book.

      list of supported image formats: there can only be one!

      This way we get support for all GdkPixbuf supported formats (like WebP if gdk-pixbuf-webp is available).

      Going forward, I think it would be best if the code used image_tools.is_image_file only, and constants.SUPPORTED_IMAGE_REGEX was folded into image_tools: this make it easier to change the library used for loading images (like using PIL, see below).

      What do you mean by "see below"? I think the solution you offer with this commit is good enough. So how are further modifications supposed to make things easier? Sorry, I just don't fully understand.

      It's better if the dependency on a particular image library is constrained to image_tools, instead of various parts of the code. This commit helps, so do the following commits.

      [wip] thumbnail tools: _thumbnail_exists; PIL -> GDK

      Same rational as above: try to reduce image library dependency to image_tools. Also avoid inconsistencies: using PIL for image information when GdkPixbuf is actually used to load images.

      I understand what you are trying to accomplish but I did not yet fully understand the code.

      It's a little more complicated with GDK than when using PIL because we can't use pixbuf_get_file_info to retrieve the image option, and so using the PixbufLoader is the only way to not load and decode the full image to retrieve those options.

      image tools: optimize load_pixbuf_size

      Right now, when creating a thumbnail, the code will load the full image, and then use the current scaling mode preferences to resize it. This can be really slow. I personally don't see the point in generating a 128x128 thumbnail with Hyperbolic scaling, when the result of gdk.pixbuf_new_from_file_at_size is good enough for this. Also, it's much faster for image formats like JPEG, or WebP: the decoder can be configured to automatically scale down the output of the decoding.

      I see. Maybe comicthumb can benefit from this, too. (Not sure, though.)

      For references, when creating 128x128 thumbnails for 132 JPEG images (~3.8MB / 4038x6057 per image):

      • subversion (scaling preference has no real impact): ~20s
      • with my branch: ~7s

      image tools: disable force use of PIL on Windows

      I was looking at using PIL (initially for WebP support), and my tests shows that it is much slower than GdkPixbuf: both on Linux and on Windows (testing with Wine). So I wonder if this still make some sense? I don't see the "GTK slowness" mentioned in the comment, maybe because I'm using GTK version 2.24.2 instead of 2.18.2. Do you have a real Windows machine to test this?

      Sorry, I did not benchmark this and I don't have access to a Windows machine.

      OK. How do you generate the Windows distribution of MComix?

      use open() instead of file()
      osd: use mcomix.constants for black/white colors (github.com)
      do not use gtk.gdk.colormap_get_system (github.com)
      use gtk.main_iteration_do instead of gtk.main_iteration (github.com)
      status bar: do not use -1 for context id
      status bar: do not use widget.set_data/get_data (github.com)
      do not use deprecated dialog.set_has_separator (github.com)
      do not use pango.SCALE_XXX constants (github.com)

      I was looking at whether updating to GTK3 is possible, and actually have a version that starts (and somewhat works) after tweaking the code a little. Those changes are needed, and still compatible with GTK2, so I think they can be pushed, what do you think?

      Interesting. I don't know much about the differences between GTK2 and GTK3 (admittedly, I don't even know GTK2 very well) so I'm glad you're dealing with this issue.
      Actually, I don't like the idea that switching to another library might break something. On the other hand, these changes don't look like they are going to break things easily. Is it okay for you to postpone these "in prevision of GTK3 support" changes for a later release?

      Sure, but the risk with those changes is really really low (especially the first 4).

      Again, thank you very much for your work. It took me a while to review all these commits. Is it possible to push only some of them to SVN? If so, I would like to choose the ones to be included in the upcoming release.

      Maybe, depends on the commits. Tell me which ones and I'll check.

       

  • Benoit Pierre

    Benoit Pierre - 2015-03-12

    Further proof that comicthumb is basically dead code:

    • it's not shipped by Ubuntu
    • on Arch Linux, the old (not supported anymore) Gconf schema method for thumbnailers is used (mime/mcomix.thumbnailer should be renamed, updated, and used instead, see fix)
    • it does not work anyway: the input argument is an encoded file:// URL (see fix)
     

  • Ark

    Ark - 2015-03-12

    I agree that preferences should only be accessed by higher level (near the GUI) code. comicthumb is also really not a priority for me (I don't use a graphical file manager). I also wonder how many MComix users use it: nobody on Windows, and on Linux, there are probably other thumbnailers available. This way was the easiest and the fastest, and ensure we get a consistent behaviour between comicthumb and MComix: same supported formats (archive+images), same cover detection.

    Agreed.

    Good idea. Small fix: I think it's better to use %s instead of concatenating the archive's path here.

    Doing it this way means the gettext translations don't need to be updated. I also think it's better to show the full path.

    Agreed, you have a point there.

    I think I got the idea why "option-like entries" need special treatment but I did not yet understand what actually happens if they occur.

    We may not get the data we asked for (when using the external ZIP executable), because the entry will be interpreted as a pattern.

    I see.

    No, the current directory contains only 24K, but the testsuite generate more test files:
    - test/files/archives/Flat.7z
    [...]
    - test/files/archives/Unicode.zip

    The total size of the directory with these new files is 240K.

    Now I see. Considering that mcomix-large.png needs 2.2MiB, I think it's okay if we add 240K in this case.

    Personal notes for setuping a Windows environment for testing. Could be added to a new HACKING readme or section in README?

    Maybe the Wiki is a better place for these kinds of notes. How about a new section in https://sourceforge.net/p/mcomix/wiki/Maintainance/?

    I'm not too fond of the Wiki, I prefer the documentation to be versioned alongside the code.

    I see. Maybe it's a good idea to create a subdir for these kinds of documents? Any way, I think another name would be more appropriate, see https://xkcd.com/932/.

    I really don't know about this part of the code. I don't use a DE, or a graphical file manager, so the drag&drop code is of no use to me. Does it not work with the actual code?

    At least on Linux, it does work. (Actually, I just installed thunar and tried a drag&drop and it worked. I did not check whether the code in mind had been executed in this case, though.) Okay, just leave it this way.

    Attempt at fixing the fact that on startup, the selected tumbnail in the thumbbar is not in view... Aligning the selection so it stays 1/4 in the view is the only way I found to make it visible, but it does change the behaviour a little. Note: this is not done on mouse click, as it would be annoying. Also, the selected thumbnail does not appear as such on startup (background color not active), so still a work in progress...

    This bug is quite annoying. However, I think it is more important to have the current image highlighted than to see it in the thumbnailer. (That is, the old bug here is better than the new one.) It would be nice to see this one fixed as well, but if it turns out to be surprisingly difficult, it might be better to return to the old behavior and postpone the fix to another release.

    I suspect that this has something to do with the rearrangement of the items in the list if some of the thumbnails turn out to be of a different size than expected, but this is merely a wild guess.

    Maybe.

    Are you able to tell whether the highlighting I mentioned is now missing due to these changes? If this is the case, I would like to postpone these changes because the missing highlighting makes the GUI look more broken than the first thumbnail being out of view.

    Not sure about this one, since we don't always show the cover: when switching from the next archive with Ctrl+PageUp => last page is shown. So maybe better to start showing the last read page, what do you think?

    Good question. I think it's better to separate the way books are being opened from this dialog. That is, for example, if you press Ctrl+PageUp, MComix should always go to the last page first. Then, if MComix wants to ask the user, the dialog appears. However, if MComix has been told to "Do not ask again", MComix should go straight to the respective page (that is, the last read page or the first page, depending on what has been chosen beforehand).

    There is an inconsistency: If you "Go -> Next archive" to a book, you get asked whether to go to the last read page or not. However, if you "Go -> Previous archive" to the very same book, you do not get asked.

    I think it's by design. I personally don't like the behaviour when switching page/book.

    Could you please describe the behavior you prefer?

    This way we get support for all GdkPixbuf supported formats (like WebP if gdk-pixbuf-webp is available).

    Going forward, I think it would be best if the code used image_tools.is_image_file only, and constants.SUPPORTED_IMAGE_REGEX was folded into image_tools: this make it easier to change the library used for loading images (like using PIL, see below).

    What do you mean by "see below"? I think the solution you offer with this commit is good enough. So how are further modifications supposed to make things easier? Sorry, I just don't fully understand.

    It's better if the dependency on a particular image library is constrained to image_tools, instead of various parts of the code. This commit helps, so do the following commits.

    (See below.)

    Same rational as above: try to reduce image library dependency to image_tools. Also avoid inconsistencies: using PIL for image information when GdkPixbuf is actually used to load images.

    I understand what you are trying to accomplish but I did not yet fully understand the code.

    It's a little more complicated with GDK than when using PIL because we can't use pixbuf_get_file_info to retrieve the image option, and so using the PixbufLoader is the only way to not load and decode the full image to retrieve those options.

    I am a bit confused. From your point of view, how are which image libraries supposed to perform which kinds of tasks? From my point of view, I would like to see Pillow doing everything it can be used for. That is, GDK/Pixbufs should be used only if really necessary (which might limit this to displaying images in the GUI).

    For references, when creating 128x128 thumbnails for 132 JPEG images (~3.8MB / 4038x6057 per image):
    - subversion (scaling preference has no real impact): ~20s
    - with my branch: ~7s

    Nice.

    I was looking at using PIL (initially for WebP support), and my tests shows that it is much slower than GdkPixbuf: both on Linux and on Windows (testing with Wine). So I wonder if this still make some sense? I don't see the "GTK slowness" mentioned in the comment, maybe because I'm using GTK version 2.24.2 instead of 2.18.2. Do you have a real Windows machine to test this?

    Sorry, I did not benchmark this and I don't have access to a Windows machine.

    OK. How do you generate the Windows distribution of MComix?

    Actually, I don't build the Windows versions, Oddegamra does. (I think I already mentioned this before.) Maybe he can explain what this "GTK slowness" is all about.

    Interesting. I don't know much about the differences between GTK2 and GTK3 (admittedly, I don't even know GTK2 very well) so I'm glad you're dealing with this issue.
    Actually, I don't like the idea that switching to another library might break something. On the other hand, these changes don't look like they are going to break things easily. Is it okay for you to postpone these "in prevision of GTK3 support" changes for a later release?

    Sure, but the risk with those changes is really really low (especially the first 4).

    Okay, agreed.

    Again, thank you very much for your work. It took me a while to review all these commits. Is it possible to push only some of them to SVN? If so, I would like to choose the ones to be included in the upcoming release.

    Maybe, depends on the commits. Tell me which ones and I'll check.

    All commits up to 233276b1883fd9741f9f21986c89a789ef86d1b4 (2015-03-12 23:20:55) excluding the commit(s) which break(s) the thumbnailer (that is, the change I marked with postponed) can go to the SVN repository.

     

  • Benoit Pierre

    Benoit Pierre - 2015-03-13

    Personal notes for setuping a Windows environment for testing. Could be added to a new HACKING readme or section in README?

    Maybe the Wiki is a better place for these kinds of notes. How about a new section in https://sourceforge.net/p/mcomix/wiki/Maintainance/?

    I'm not too fond of the Wiki, I prefer the documentation to be versioned alongside the code.

    I see. Maybe it's a good idea to create a subdir for these kinds of documents? Any way, I think another name would be more appropriate, see https://xkcd.com/932/.

    HACKING? It's a pretty common use for this, no?

    :::shell
> cd ~/progs/src && print -l **/*HACKING*
ao/HACKING
bzr/doc/developers/HACKING.txt
dulwich/HACKING
dvc/docs/HACKING
gemrb/HACKING
morituri/HACKING
qemu/HACKING
ranger/HACKING.md
razer/HACKING.md
rockbox/apps/plugins/rockboy/HACKING
vimperator/HACKING

    I really don't know about this part of the code. I don't use a DE, or a graphical file manager, so the drag&drop code is of no use to me. Does it not work with the actual code?

    At least on Linux, it does work. (Actually, I just installed thunar and tried a drag&drop and it worked. I did not check whether the code in mind had been executed in this case, though.) Okay, just leave it this way.

    Apparently file://hostname/path is valid: file URI scheme

    Not sure about this one, since we don't always show the cover: when switching from the next archive with Ctrl+PageUp => last page is shown. So maybe better to start showing the last read page, what do you think?

    Good question. I think it's better to separate the way books are being opened from this dialog. That is, for example, if you press Ctrl+PageUp, MComix should always go to the last page first. Then, if MComix wants to ask the user, the dialog appears. However, if MComix has been told to "Do not ask again", MComix should go straight to the respective page (that is, the last read page or the first page, depending on what has been chosen beforehand).

    There is an inconsistency: If you "Go -> Next archive" to a book, you get asked whether to go to the last read page or not. However, if you "Go -> Previous archive" to the very same book, you do not get asked.

    I think it's by design. I personally don't like the behaviour when switching page/book.

    Could you please describe the behavior you prefer?

    Here is how I handle it in my comic viewer (no last read page support):

    • navigate out of the page (scrolling at the end): switch to next page (start: top-left or top-right if manga mode is on), if on the last page, then the next page is the next archive first page
    • navigate out of the page (scrolling back at the top): switch to previous page (end: bottom-right or bottom-left if manga mode is on), if on the first page, then the previous page is the previous archive last page
    • next/previous page: switch to next/previous page (start), next archive first page if next on last page, previous archive last page if previous on first page
    • next/previous archive: switch to first page (start) of next/previous archive

    With last read page support, maybe we should always ask the user when switching to an archive? (If there is a last read page and we're not already on it of course).

    Same rational as above: try to reduce image library dependency to image_tools. Also avoid inconsistencies: using PIL for image information when GdkPixbuf is actually used to load images.

    I understand what you are trying to accomplish but I did not yet fully understand the code.

    It's a little more complicated with GDK than when using PIL because we can't use pixbuf_get_file_info to retrieve the image option, and so using the PixbufLoader is the only way to not load and decode the full image to retrieve those options.

    I am a bit confused. From your point of view, how are which image libraries supposed to perform which kinds of tasks? From my point of view, I would like to see Pillow doing everything it can be used for. That is, GDK/Pixbufs should be used only if really necessary (which might limit this to displaying images in the GUI).

    I used to think the same, but after investigating Pillow has an alternative for WebP support (on Linux gdk-pixbuf-webp can be used) I know think that GdkPixbuf should be preferred over it. Here is why: my experience is that using PIL/Pillow make the UI more sluggish. I think the problem is, with PIL/Pillow, we shift some treatment from C to Python, and with Python threading model, the result is a perceptible reduction of MComix interface reactivity.

    You can test for yourself with this patch:

    diff --git i/mcomix/image_tools.py w/mcomix/image_tools.py
index 193d95c..445700f 100644
--- i/mcomix/image_tools.py
+++ w/mcomix/image_tools.py
@@ -6,10 +6,13 @@ import gtk
 import PIL.Image as Image
 import PIL.ImageEnhance as ImageEnhance
 import PIL.ImageOps as ImageOps
+from StringIO import StringIO

 from mcomix.preferences import prefs
 from mcomix import constants

+USE_PIL = True
+
 def fit_pixbuf_to_rectangle(src, rect, rotation):

     if rotation in (90, 270):
@@ -243,11 +246,17 @@ def pixbuf_to_pil(pixbuf):

 def load_pixbuf(path):
     """ Loads a pixbuf from a given image file. """

+    if USE_PIL:
+        return pil_to_pixbuf(Image.open(path))
     return gtk.gdk.pixbuf_new_from_file(path)

 def load_pixbuf_size(path, width, height):
     """ Loads a pixbuf from a given image file and scale it to fit
     inside (width, height). """

+    if USE_PIL:
+        im = Image.open(path)
+        im.thumbnail((width, height))
+        return pil_to_pixbuf(im)
     format, src_width, src_height = get_image_info(path)
     if src_width <= width and src_height <= height:
         src = gtk.gdk.pixbuf_new_from_file(path)
@@ -265,6 +274,8 @@ def load_pixbuf_size(path, width, height):

 def load_pixbuf_data(imgdata):
     """ Loads a pixbuf from the data passed in <imgdata>. """

+    if USE_PIL:
+        return pil_to_pixbuf(Image.open(StringIO(imgdata)))
     loader = gtk.gdk.PixbufLoader()
     loader.write(imgdata, len(imgdata))
     loader.close()

    Again, thank you very much for your work. It took me a while to review all these commits. Is it possible to push only some of them to SVN? If so, I would like to choose the ones to be included in the upcoming release.

    Maybe, depends on the commits. Tell me which ones and I'll check.

    All commits up to 233276b1883fd9741f9f21986c89a789ef86d1b4 (2015-03-12 23:20:55) excluding the commit(s) which break(s) the thumbnailer (that is, the change I marked with postponed) can go to the SVN repository.

    OK, so here is what I plan to push to Subversion: incoming!

    It's everything minus:

    • the archlinux stuff
    • the thumbnails bar changes
    • the fix for "don't act on click to focus" handling (dependent on the thumbnails bar changes)
    • the HACKING doc
    • WIP code for comicthumb: archive.ask_for_password

    Note that this include removing the "force PIL on Windows" code, so maybe we could ask Oddegamra to create a candidate release version for Windows and post a news item asking our Windows users for feedback?

     

  • Ark

    Ark - 2015-03-13

    HACKING? It's a pretty common use for this, no? […]

    Apparently, yes. However, I thought that some inexperienced users might get the wrong idea when reading the name of this document. So, maybe it's better to move it to a subdir, e.g. docs/HACKING. (Sorry, I know that it sounds a bit stupid.)

    Apparently file://hostname/path is valid: file URI scheme

    I see. Well, I was not sure how localhost must be resolved but according to Wikipedia, localhost is reserved and always points to a loopback device. (If I got it right, it also means that it does not need to be listed in /etc/hosts to be resolved appropriately.) So it seems that we can leave it the way it is but I still think that a new function would make code cleaner. (On the other hand, I cannot test it in Windows, only in Wine, so this might break things albeit stackoverflow suggests that it should work fine.)

    I think it's by design. I personally don't like the behaviour when switching page/book.

    Could you please describe the behavior you prefer?

    Here is how I handle it in my comic viewer (no last read page support):

    • navigate out of the page (scrolling at the end): switch to next page (start: top-left or top-right if manga mode is on), if on the last page, then the next page is the next archive first page
    • navigate out of the page (scrolling back at the top): switch to previous page (end: bottom-right or bottom-left if manga mode is on), if on the first page, then the previous page is the previous archive last page
    • next/previous page: switch to next/previous page (start), next archive first page if next on last page, previous archive last page if previous on first page
    • next/previous archive: switch to first page (start) of next/previous archive

    With last read page support, maybe we should always ask the user when switching to an > archive? (If there is a last read page and we're not already on it of course).

    Thank you for the explanation. So the only(?) difference is that, currently, switching to the previous archive opens its last page rather than its first page. I have an idea but it needs some more time so let's postpone the discussion for a later release.

    I am a bit confused. From your point of view, how are which image libraries supposed to perform which kinds of tasks? From my point of view, I would like to see Pillow doing everything it can be used for. That is, GDK/Pixbufs should be used only if really necessary (which might limit this to displaying images in the GUI).

    I used to think the same, but after investigating Pillow has an alternative for WebP support (on Linux gdk-pixbuf-webp can be used) I know think that GdkPixbuf should be preferred over it. Here is why: my experience is that using PIL/Pillow make the UI more sluggish. I think the problem is, with PIL/Pillow, we shift some treatment from C to Python, and with Python threading model, the result is a perceptible reduction of MComix interface reactivity.

    You can test for yourself with this patch: […]

    I did not yet test it but let's postpone the discussion as well.

    OK, so here is what I plan to push to Subversion: incoming!

    It's everything minus:
    - the archlinux stuff
    - the thumbnails bar changes
    - the fix for "don't act on click to focus" handling (dependent on the thumbnails bar changes)
    - the HACKING doc
    - WIP code for comicthumb: archive.ask_for_password

    Note that this include removing the "force PIL on Windows" code, so maybe we could ask Oddegamra to create a candidate release version for Windows and post a news item asking our Windows users for feedback?

    The incoming branch looks promising, thank you. Please push it to SVN now.

    A candidate release sounds interesting but I don't want to ask Oddegamra too often for a release. I also think that this "force PIL on Windows" issue might need a specific setup Oddegamra is already aware of. Maybe we should ask him for clarification first. While he is at it, he might also want to check how to deal with czipfile on Windows.

    That is, instead of a candidate release, we should ask him for testing these Windows-specific changes. If he says that everything runs fine, we will later ask him to prepare a new release (especially for Windows). What do you think?

    Note to self: Implement path_to_url function before asking Oddegamra for testing.

     

  • Benoit Pierre

    Benoit Pierre - 2015-03-13

    The incoming branch looks promising, thank you. Please push it to SVN now.

    Done.

    A candidate release sounds interesting but I don't want to ask Oddegamra too often for a release. I also think that this "force PIL on Windows" issue might need a specific setup Oddegamra is already aware of. Maybe we should ask him for clarification first.

    Right.

    While he is at it, he might also want to check how to deal with czipfile on Windows.

    There is a Windows installer available here.

    That is, instead of a candidate release, we should ask him for testing these Windows-specific changes. If he says that everything runs fine, we will later ask him to prepare a new release (especially for Windows). What do you think?

    Agreed.

    While we'are on the subject of the Windows release, I wonder if we should include the licences for some of the libraries? (UnRAR.dll, GTK, ...). Also, maybe we could bundle MuPDF so PDF support is working out of the box?

     

  • Ark

    Ark - 2015-03-14

    What is your opinion on https://sourceforge.net/p/mcomix/bugs/73/?limit=25&page=1#0e07? (Maybe I have missed something or there is a misunderstanding.)

    I also edited the table about empty archive handling so it always says "Yes" in "Can navigate from". Currently, you can enter an empty archive with Ctrl+PgDown but you cannot leave it this way, which makes the navigation feel broken.

     

  • Benoit Pierre

    Benoit Pierre - 2015-03-14

    I have pushed a fix for page navigation in empty archives. That live the case of empty directories.

    I see that you pushed a change to mime/comicbook.schemas, I forgot to do it, but we should really just remove it: this method of handling thumbnails has been deprecated for a long time.

     

  • Ark

    Ark - 2015-03-14

    I have pushed a fix for page navigation in empty archives. That live the case of empty directories.

    Thank you. I still think that the user should be able to switch to empty directories. (I wonder why the code makes it possible for MComix to treat archives differently anyway.) It would make the behavior more consistent. For example, if there is a broken image, MComix indicates that it is broken and displays a dummy image rather than omitting the image entirely. Books should be treated similarly.

    I see that you pushed a change to mime/comicbook.schemas, I forgot to do it, but we should really just remove it: this method of handling thumbnails has been deprecated for a long time.

    Admittedly, I just thought that the change might fix more things than it breaks. Maybe this file has been kept for backward compatibility? I don't know.

     

  • Benoit Pierre

    Benoit Pierre - 2015-03-14

    I still think that the user should be able to switch to empty directories. (I wonder why the code makes it possible for MComix to treat archives differently anyway.) It would make the behavior more consistent. For example, if there is a broken image, MComix indicates that it is broken and displays a dummy image rather than omitting the image entirely. Books should be treated similarly.

    How about something along those lines:

    diff --git i/mcomix/file_handler.py w/mcomix/file_handler.py
index 7d352d0..bbeaea6 100644
--- i/mcomix/file_handler.py
+++ w/mcomix/file_handler.py
@@ -118,6 +118,7 @@ class FileHandler(object):
     if error_message:
         self._window.statusbar.set_message(error_message)
         self._window.osd.show(error_message)

+        self.file_load_failed = False
         return False

     self.filelist = filelist
@@ -169,7 +170,6 @@ class FileHandler(object):
     self._window.imagehandler._image_files = image_files

     if not image_files:

-        self.file_load_failed = True
         msg = _("No images in '%s'") % os.path.basename(self._current_file)
         self._window.statusbar.set_message(msg)
         self._window.osd.show(msg)
@@ -177,8 +177,6 @@ class FileHandler(object):
         self._window.uimanager.set_sensitivities()

     else:

-        self.file_load_failed = False
-
         if self.archive_type is None:
         # If no extraction is required, mark all files as available.
         self.file_available(self.filelist)
@@ -306,14 +304,6 @@ class FileHandler(object):
     elif not os.access(path, os.R_OK):
         return _('Could not open %s: Permission denied.') % path


-    elif archive_type is None and len(filelist) == 0:
-        return _("No images in '%s'") % path
-
-    elif (archive_type is None and
-        not image_tools.is_image_file(path) and
-        len(filelist) == 0):
-        return _('Could not open %s: Unknown file type.') % path
-
     else:
         return None

@@ -547,19 +537,22 @@ class FileHandler(object):
         listmode = file_provider.FileProvider.IMAGES

     current_dir = self._file_provider.get_directory()

-    while self._file_provider.next_directory():
-        files = self._file_provider.list_files(listmode)
-
-        if len(files) > 0:
-        self.close_file()
-        self._window.scroll_to_predefined(
-            (constants.SCROLL_TO_START,) * 2, constants.FIRST_INDEX)
-        self.open_file(files[0], keep_fileprovider=True)
-        return True
-
-    # Restore current directory if no files were found
-    self._file_provider.set_directory(current_dir)
-    return False
+    if not self._file_provider.next_directory():
+        # Restore current directory if no files were found
+        self._file_provider.set_directory(current_dir)
+        return False
+
+    files = self._file_provider.list_files(listmode)
+    self.close_file()
+    self._window.scroll_to_predefined(
+        (constants.SCROLL_TO_START,) * 2, constants.FIRST_INDEX)
+    if len(files) > 0:
+        path = files[0]
+    else:
+        path = self._file_provider.get_directory()
+    self.open_file(path, keep_fileprovider=True)
+    return True
+

     def open_previous_directory(self, *args):
     """ Opens the previous sibling directory of the current file, as specified by
@@ -571,19 +564,21 @@ class FileHandler(object):
         listmode = file_provider.FileProvider.IMAGES

     current_dir = self._file_provider.get_directory()

-    while self._file_provider.previous_directory():
-        files = self._file_provider.list_files(listmode)
-
-        if len(files) > 0:
-        self.close_file()
-        self._window.scroll_to_predefined(
-            (constants.SCROLL_TO_END,) * 2, constants.LAST_INDEX)
-        self.open_file(files[-1], -1, keep_fileprovider=True)
-        return True
-
-    # Restore current directory if no files were found
-    self._file_provider.set_directory(current_dir)
-    return False
+    if not self._file_provider.previous_directory():
+        # Restore current directory if no files were found
+        self._file_provider.set_directory(current_dir)
+        return False
+
+    files = self._file_provider.list_files(listmode)
+    self.close_file()
+    self._window.scroll_to_predefined(
+        (constants.SCROLL_TO_END,) * 2, constants.LAST_INDEX)
+    if len(files) > 0:
+        path = files[-1]
+    else:
+        path = self._file_provider.get_directory()
+    self.open_file(path, -1, keep_fileprovider=True)
+    return True

     def file_is_available(self, filepath):
     """ Returns True if the file specified by "filepath" is available
     

  • Ark

    Ark - 2015-03-14

    Thank you for the patch but for some reason, I cannot apply it:

    $ patch -p1 < ../empty_book_navigation.patch 
patching file mcomix/file_handler.py
Hunk #1 FAILED at 118.
Hunk #2 FAILED at 169.
Hunk #3 FAILED at 177.
Hunk #4 FAILED at 306.
Hunk #5 FAILED at 547.
Hunk #6 FAILED at 571.
6 out of 6 hunks FAILED -- saving rejects to file mcomix/file_handler.py.rej

    (Using svn does not work either.) Maybe it's because the diff is broken thanks to this board's quirks, I don't know. Could you please create a new patch and attach it as a file to the post?

    Concerning [r1209]: Thank you. Apparently, actions like Ctrl+N and Ctrl+P are broken as well.

     

    Related

    Commit: [r1209]

  • Benoit Pierre

    Benoit Pierre - 2015-03-14

    Could you please create a new patch and attach it as a file to the post?

    Here you go.

    Concerning [r1209]: Thank you. Apparently, actions like Ctrl+N and Ctrl+P are broken as well.

    Since may 2013 apparently... :P

     

    Related

    Commit: [r1209]

    file-handler-empty-dirs.patch

  • Benoit Pierre

    Benoit Pierre - 2015-03-14

    We also keep the same file provider after closing an archive. It is the behavior we want? (Ctrl+N/Ctrl+P will go to the next/previous directory of the previously opened archive)

     

  • Ark

    Ark - 2015-03-14

    Good question. (It never occured to me to hit Ctrl+P after closing a book.) I think it's better to reset everything so it behaves as if no book had ever been opened. (I think of it as a privacy issue.) In other words, commands like Ctrl+P should be disabled and should not work because they do not know what to do (or where to go to).

    Thank you for the patch file. The indentation was incorrect when I tried to create a patch file from what you posted earlier. Please always supply patches by attaching them.

     
<< < 1 2 3 > >> (Page 2 of 3)

Log in to post a comment.

MongoDB Logo MongoDB