MComix / Patches / #41 new batch of changes for inclusion

Ark - 2015-04-05

Thank you very much for your help.

I just finished testing the current changes you made to fix the rotation issue. If I observed it correctly, it seems that there is still a bug: If you "flip horizontally" or "flip vertically" (or both) and the final rotation turns out to be either 90 degrees or 270 degrees (no matter how it happens, but I did not test Exif influences here), then the result turns out to be broken. It does not depend on other settings. That is, it is broken in any combination of manga mode, western mode, single page mode, double page mode, any auto rotation being turned on or off, or manual rotation, etc.

I am going to test the other changes as well.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-05

Now that I have thought about it a bit more: Maybe the transformation is actually done the "correct" way, depending on how you define "correct". I assumed that it is (or rather should be) done this way:

Apply Exif rotation.

Apply vertical/horizontal flipping.

Apply auto rotation.

Apply manual rotation.

But it seems that currently (that is, assuming the current behavior on branch rotation) all rotations are applied first and mirroring is performed last. The strange thing about this is that rotating manually several times looks awkward.

I guess it depends on what you are trying to achieve with flipping. I think the order I just described above is more meaningful if you think of mirroring as some kind of "fixing" images that are mirrored for whatever reason. Also, if manual rotation is performed last, you can show images to other people more easily in a tablet-like setup.

What is your opinion here?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-05

I did not touch that part. I personally would tend to prefer for the vertical/horizontal flip to be done last. What about the fact that it is applied individually? Do you agree with the fact that it should be applied to the virtual page? E.g.:

double page mode: p1 p2

double page mode with horizontal flip: 2p 1p

double page mode with manga mode: p2 p1

double page mode with manga mode and horizontal flip: 1p 2p

I also pushed a few more changes:

fixup! library: use tighter layout for book area

The earlier version made it hard to see which book was selected.

fixup! [wip] better "use pil" version

Fix image_tools.get_image_info to use PIL when applicable.

image tools: add get_supported_formats

Return a dictionary mapping: { format name: (mime types, extensions) }

archive tools: add pdf_available/get_supported_formats

archive tools: add pdf_available

archive tools: add get_supported_formats

file chooser dialogs: rework filters code

file chooser dialogs: factorize image filters code

file chooser dialogs: rework image filters code

file chooser dialogs: rework archive filters code

This build the image filters using the list of supported formats returned by image_tools.get_supported_formats, so it work consistently when PIL is used.

Similarly, the archive filters are built with the list of supported formats returned by archive_tools.get_supported_formats.

Note: this add the missing PDF support.

update translations

messages: update translations

messages: update french translation

Update the translations for the previous changes (filters). I also took this opportunity to update the french translation.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-05

Note: I did not yet read your most recent post or changes. This post is about the bpierre branch up to 1de8a490edf8e5d926a03420041fae478bfa3401.

I am really amazed at all these changes provided in this ticket, both the quality and the quantity are just awesome (at least as far as I can tell). Thank you very much. The changes seem to fix tons of issues while reducing LOC.

Okay, here are some more detailed comments on some of the changes:

Thumbnailer bugs:

If you run into an empty archive or similar cases where the thumbnailer is hidden and the OSD displays "Could not load image" or a similar error message, you can navigate horizontally through empty space in the main area using your cursor keys. The size of this space depends on the size of the thumbnails (according to the current prefs) so I guess some part of the code does not receive an update stating that there is currently no thumbnailer visible anymore.

A focus issue: Open a book at the first page. Then, open the Prefs dialog and scroll down in the thumbnailer while the Prefs dialog stays focused (works at least in my window manager) so the currently selected image is far from being visible in the thumbnailer. Now click on a thumbnail you can see now in the thumbnailer, e.g. page 100. Instead of going to page 100 (or simply switching focus without actually switching to page 100), the thumbnailer is scrolled back to the currently selected image.

A Thumbnail bug (possibly not really our bug):

The following bug can be reproduced using a file containing only NUL bytes, e.g.

dd if=/dev/zero of=test_nul.jpg count=1 # 512 NUL bytes

Such a "NUL image file" is broken but it does not get any thumbnail in the thumbnailer. It seems that a zero-width zero-height image is being created as a "thumbnail" instead. The "broken image" icon should be used as a replacement here. (Note that completely empty files already get this replacement.) Also, if you use "File -> Properties" on such a file, you'll get an exception and some error messages. (The exception is not raised if the Properties dialog has already been opened beforehand.)

I think this change should not be made:
https://github.com/benoit-pierre/mcomix/commit/6d645433b792083e94e224d20e4097934120e4af
Reasoning: The old implementation does not need floating point arithmetic and also works if n == 0. (The new implementation raises an exception in this case.) If there is anything to improve in this function, you might want to add n = abs(n) at the beginning of the function to make it work with negative numbers as well.

I like how you make sure that the thumbnailer is wide enough to take all digits. (The old computation bugged me, too.) Also, with the new code, it seems that all thumbnails get the same "thumbnail size" according to the prefs. (There is a race condition in the old code that could lead to thumbnails being of different "thumbnail sizes" if you change this pref fast enough using the spinner, for example.)

You might want to present the "wine helper script" to Oddegamra so he can check whether the result works and looks as intended on a real Windows machine.

About this change: https://github.com/benoit-pierre/mcomix/commit/1de8a49

I somehow lost track of when PIL and when GDK is being used. What I'm wondering, however, is whether it is a good idea to expose use_pil to to the user and in the prefs file. I think the value of this setting should be determined when MComix is starting up and should not be exposed to the user (neither in the prefs dialog nor in the prefs file).

However, if you think that the user should be able to select the image loader in future versions, it might be better to introduce a preference-ordered list instead of a boolean value, e.g. ["pil", "gdk"] if PIL should be preferred over GDK, or ["gdk"] to enforce usage of GDK and disable any other image loaders. This way, the prefs might be more compatible to future changes, e.g. if a third image loader is being introduced.

I still think, though, that at least for the upcoming release, this setting should not be exposed at all. Instead, we should rely on Oddegamra's evaluation and keep the "old" behavior, at least for now. Otherwise, we might need to wait for feedback, which takes time and delays things even further.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-05

Once more on the GDK-or-PIL issue:

How about this: We introduce a pref "image loaders" that maps to a list and defaults to

"image loaders": ["auto"]

with the following semantics:

If there is anything wrong with this list (that is, if it is empty or not present, or contains unknown values, for example), reset it to the default value.

Whenever an image has to be loaded, try to use the image loader that is mentioned first in the list first. If it fails to load an image, try the next one, and so on. If no image loader manages to load the image, fail for this particular image.

The special value "auto" allows MComix to determine a suitable image loader by itself (which might take anything into account, such as the OS in use, file size, or other image loaders present in the list to skip them).

Each value may appear at most once in the list.

Examples:

["gdk", "pil"] prefers GDK over PIL.

["pil"] forces using PIL and disables GDK.

["pil", "auto"] currently behaves like ["pil", "gdk"] on all systems.

["auto", "pil"] currently behaves like ["gdk", "pil"] on all systems.

["auto"] currently behaves like ["pil", "gdk"] on Windows and ["gdk", "pil"] on other systems.

This way, we can go for a new release without the need for a beta and, at the same time, allow (Windows) users to try out different image loaders. We could also add widgets to the Preferences dialog later, or, if this pref is of no use anymore, always fall back to ["auto"].

What do you think?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-05

Thumbnailer bugs:

If you run into an empty archive or similar cases where the thumbnailer is hidden and the OSD displays "Could not load image" or a similar error message, you can navigate horizontally through empty space in the main area using your cursor keys. The size of this space depends on the size of the thumbnails (according to the current prefs) so I guess some part of the code does not receive an update stating that there is currently no thumbnailer visible anymore.

Apparently not a regression: I get the same behaviour going back to at least 1.00. We'll look into it.

One transition I would really like to improve is when toggling fullscreen mode.

A focus issue: Open a book at the first page. Then, open the Prefs dialog and scroll down in the thumbnailer while the Prefs dialog stays focused (works at least in my window manager) so the currently selected image is far from being visible in the thumbnailer. Now click on a thumbnail you can see now in the thumbnailer, e.g. page 100. Instead of going to page 100 (or simply switching focus without actually switching to page 100), the thumbnailer is scrolled back to the currently selected image.

Not a bug, a feature! This is the "don't act on click to focus" code at work.

A Thumbnail bug (possibly not really our bug):

The following bug can be reproduced using a file containing only NUL bytes, e.g.

dd if=/dev/zero of=test_nul.jpg count=1 # 512 NUL bytes

After investigation:

the thumbnailer code check if it's an archive file: I'm thinking of adding a setting to the thumbnailer to disable archive checks in this case

tarfile.is_tarfile(path) returns True: the file is empty, so no cover can be found

the image handler code only uses the "broken image" icon in case of exception

I think this change should not be made:
https://github.com/benoit-pierre/mcomix/commit/6d645433b792083e94e224d20e4097934120e4af
Reasoning: The old implementation does not need floating point arithmetic and also works if n == 0. (The new implementation raises an exception in this case.) If there is anything to improve in this function, you might want to add n = abs(n) at the beginning of the function to make it work with negative numbers as well.

Doh! The code should definitively handle n <= O. I don't think using floating point arithmetic is an issue. What matters most (to me) is to simplify the code.

You might want to present the "wine helper script" to Oddegamra so he can check whether the result works and looks as intended on a real Windows machine.

I'll send him a message.

I somehow lost track of when PIL and when GDK is being used. What I'm wondering, however, is whether it is a good idea to expose use_pil to to the user and in the prefs file. I think the value of this setting should be determined when MComix is starting up and should not be exposed to the user (neither in the prefs dialog nor in the prefs file).

I was thinking of using it only for a candidate/test release, to ask Windows users for feedback.

But then again, I could see value in exposing the setting to the user. Rational:

the image formats supported by GDK and PIL are not the same

support in one library can be bugged: e.g. WebP support in Pillow 2.7 leaks memory like crazy

one of the library might be easier to update than the other for the user/us depending on the environment

If we keep the setting (and expose it to the user) than the following should be done:
- change the setting to a string: "image loader", with possible values "pil" or "gdk"
- when exposing it to the user in the preferences dialog: show the version of each library, and the corresponding list of supported formats

It might be better to introduce a preference-ordered list instead of a boolean value, e.g. ["pil", "gdk"] if PIL should be preferred over GDK, or ["gdk"] to enforce usage of GDK and disable any other image loaders.

Another possibility, but it would make the code even more complicated. Does that mean the code should attempt to load an image first with PIL, and then with GDK if the setting is set to ["pil", "gdk"]? I'm worried about the possible performance implications.

Note: I think part of the performance differences I was seeing between PIL and GDK was due to the bug in the code to reap zombie process. GDK still seems to be a little faster (at least on Linux), but it's not as clear cut as with the bug screwing with Python scheduling. GTK3 seems faster than GTK2 on Windows (with Wine). I should make a benchmark...

All in all I'm okay with releasing with "use pil"/USE_PIL set to True on Windows.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-05

Not a bug, a feature! This is the "don't act on click to focus" code at work.

For the most part only, I think. The only-flip-pages-if-already-focused part is okay. However, the scroll-thumbnailer-back-to-the-current-image-while-gaining-focus part is not really nice.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-05

Not a bug, a feature! This is the "don't act on click to focus" code at work.

For the most part only, I think. The only-flip-pages-if-already-focused part is okay. However, the scroll-thumbnailer-back-to-the-current-image-while-gaining-focus part is not really nice.

Agreed, I pushed a fix for this.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-06

Now that I have thought about it even more, I wonder whether it's better to postpone the "use pil" issue to a later release. Reasons:

Oddegamra told me (almost a month ago, actually) that MComix uses PIL (Pillow) to load not only images but also GUI icons due to the massive slowdown, see https://osdir.com/ml/gtk-list/2010-02/msg00125.html That is, the user might end up screwing the GUI if image loaders can be chosen manually. (Admittedly, we could fix the GUI part, but that would make things even more complicated.)

We did not yet figure out when which imaging library should or must be used. If I recall correctly, certain parts in MComix need Pillow anyway. Converting between different image representations might not only slow down everything but also wastes memory. This is related to [feature-requests:#92].

I don't know whether the slowliness also applies to GTK3, but if we are going to make a transition to GTK3 anyway, we should check this first. Maybe the transition itself fixes this issue.

Archive extraction was slow. Thanks to you, this is not the case anymore. If image loading and processing itself is that slow in the most recent release, I guess that there would already be a ticket for it.

Thanks to your huge code refactoring, it might be possible to improve image processing itself now. Maybe we should start from there, analyzing and benchmarking first, while keeping in mind the GTK3 transition, so we can tell what the overall architecture might look like in the future. This way, we might be able to see when or where we are forced to use certain libraries, and when or where we do actually have a choice.

Related

Feature Requests: #92
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-06

I just had a look at the other changes up to 9039372958366f59d566096223bbca56d55eb592. Just as the changes made earlier here, BIG LIKE! :-)

About https://github.com/benoit-pierre/mcomix/commit/64898e9:
The supported_formats line for BMP uses a tuple instead of a list. Should be made consistent with the other elements. Also, a small refactoring would be nice: Remove the early return if USE_PIL and join this path with the final return statement (i.e. add else).

Is there a way to add separators to the file filter drop-down box in the "Open" dialog? With or without such separators, I think reordering them a bit might be helpful, like this:

All files

All supported formats (should be selected by default)

All supported archives

All supported images

(Separator)

(enumerate all supported archive formats here)

(Separator)

(enumerate all supported image formats here)

About the transformation issue: You are right. Sometimes, there are actual double-page images (i.e. single images that show two pages of a book next to each other). To make the transformations consistent with these cases, treating the result of the "double page" mode as a whole might be actually necessary. (The Exif-related transformation, i.e. rotation, should be performed on the individual images first, of course.)

I am not sure about the order, though. The main drawback of "rotation first, mirroring last", in my opinion, is that the rotation seems to be performed the other way around from the user's perspective. That is, for example, if "flip vertically" is currently turned on and the user selects "rotate CW", the image seems to be rotated CCW instead. That's why I prefer "mirroring first, rotation last".

A third option might be performing a single transformation on what is currently being displayed to the user ("add transformation"). While this might be the most intuitive way for the user, however, it might need more changes to the code.

Note that all three descriptions, "rotation then mirroring", "mirroring then rotation" and "add transformation" are equally powerful, since the all describe the same set of eight possible final transformations. Therefore, the (from my point of view most intuitive) "add transformation" approach might be the way to go. I am afraid that modifying the code for this to happen might take quite a while. On the other hand, we should not change the behavior of MComix twice here since users usually want to get used to a software and don't want to learn the ropes of it anew all the time.

Which one of these approaches do you prefer? I really like the last one.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-07

Not only concerning that "third option" as mentioned above: Maybe we should switch to matrices to represent linear maps in general, converting them to other representations only if necessary. The reason is that matrices are more compact, more general, and many libraries (including Pillow) support or even need them anyway.

I attached a couple of functions that might help here, including an example demonstrating how to use them. (I hope that they work. Depending on how the coordinate system in use is oriented and which rotation direction is preferred, the seq_table must be designed accordingly.)

What do you think about it?

linear.py

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-07

Note that the functions are only designed to work with those 8 maps. Purely academic note: Function composition and these 8 maps form the finite dihedral group of order 8: https://en.wikipedia.org/wiki/Examples_of_groups#dihedral_group_of_order_8

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-08

Now that I have thought about it even more, I wonder whether it's better to postpone the "use pil" issue to a later release. Reasons:

Oddegamra told me (almost a month ago, actually) that MComix uses PIL (Pillow) to load not only images but also GUI icons due to the massive slowdown, see https://osdir.com/ml/gtk-list/2010-02/msg00125.html That is, the user might end up screwing the GUI if image loaders can be chosen manually. (Admittedly, we could fix the GUI part, but that would make things even more complicated.)

2010 is ancient; admittedly the last release for PyGtk is only from 2012.

Also, I looked at the code for MComix-1.01, and icons are not loaded by PIL: image_tools.load_pixbuf_data is used, and its implementation uses GDK.

We did not yet figure out when which imaging library should or must be used. If I recall correctly, certain parts in MComix need Pillow anyway. Converting between different image representations might not only slow down everything but also wastes memory. This is related to [feature-requests:#92].

To be clear: this is not about replacing some library for the other; the code still expects a GDK pixbuf after loading, and still switches to PIL for some operations. This is only about which one is used for decoding.

I don't know whether the slowliness also applies to GTK3, but if we are going to make a transition to GTK3 anyway, we should check this first. Maybe the transition itself fixes this issue.

Archive extraction was slow. Thanks to you, this is not the case anymore. If image loading and processing itself is that slow in the most recent release, I guess that there would already be a ticket for it.

It's not about image loading being slow, rather it's about the possibility of making it faster.

Thanks to your huge code refactoring, it might be possible to improve image processing itself now. Maybe we should start from there, analyzing and benchmarking first, while keeping in mind the GTK3 transition, so we can tell what the overall architecture might look like in the future. This way, we might be able to see when or where we are forced to use certain libraries, and when or where we do actually have a choice.

The GTK3 transition does not change the architecture: 90% of it was just renaming stuff.

Is there a way to add separators to the file filter drop-down box in the "Open" dialog? With or without such separators, I think reordering them a bit might be helpful, like this:

All files

All supported formats (should be selected by default)

All supported archives

All supported images

(Separator)

(enumerate all supported archive formats here)

(Separator)

(enumerate all supported image formats here)

I was thinking the same thing:

add an "All supported formats" entry, selected by default, since it's the best mode for 99% of the use cases

find a way to make the list clearer, I don't know if adding a separator is possible, we'll look into it (I tried to see if bolding an entry is possible: it's not)

I am not sure about the order, though. The main drawback of "rotation first, mirroring last", in my opinion, is that the rotation seems to be performed the other way around from the user's perspective. That is, for example, if "flip vertically" is currently turned on and the user selects "rotate CW", the image seems to be rotated CCW instead. That's why I prefer "mirroring first, rotation last".

Except when applying a mirror tranformation when an automatic rotation is already active, then it's the other way round: "rotation first, mirroring last" is more intuitive.

Note that all three descriptions, "rotation then mirroring", "mirroring then rotation" and "add transformation" are equally powerful, since the all describe the same set of eight possible final transformations. Therefore, the (from my point of view most intuitive) "add transformation" approach might be the way to go. I am afraid that modifying the code for this to happen might take quite a while. On the other hand, we should not change the behavior of MComix twice here since users usually want to get used to a software and don't want to learn the ropes of it anew all the time.

Which one of these approaches do you prefer? I really like the last one.

I tend to agree, but I need to think about this more.

Not only concerning that "third option" as mentioned above: Maybe we should switch to matrices to represent linear maps in general, converting them to other representations only if necessary. The reason is that matrices are more compact, more general, and many libraries (including Pillow) support or even need them anyway.

I attached a couple of functions that might help here, including an example demonstrating how to use them. (I hope that they work. Depending on how the coordinate system in use is oriented and which rotation direction is preferred, the seq_table must be designed accordingly.)

What do you think about it?

Sure, I'm all in favor of simplifying the implementation, but we need to figure the specification first.

In the mean time, I reordered the commits in my branch to move the more "controversial" stuff toward the end (you can ignore the last 9 changes), and made a few more changes.

Here is a list of the commits in the right order (since Github does not handle rebase very well: commits are reordered chronologically):

8e066b5: minor simplification in double page handling

bd6e99c: main: minor cleanup

4e638be: use super for base class(es) init

598d589: file handler: remove unnecessary scrolling calls

cfd2ea0: file handler: remove _must_call_draw hack

a0f5a7c: properties dialog: handle possible os.stat exception

dde9d4b: message dialog: small cleanup

90e8e1e: dialogs: fix parenting issue

fe35f75: library: minor cleanup

c9ce380: lens: fix border handling

287f76d: osd: remove timer when clear is called

ce11831: lens: clear osd when starting to use the lens

c04e047: main: fix weird transition when flipping page

48cdf04: image handler: fix set_page documentation

73edb76: tools: simplify vector_add, vector_sub, and vector_opposite

58fde8b: constants: fix version compliance with PEP440

d608cca: move constants.SUPPORTED_IMAGE_REGEX to image_tools

2f48364: image tools: add get_supported_formats

8fe0064: archive tools: add pdf_available

7ceb7da: archive tools: add get_supported_formats

cdb1630: file chooser dialogs: factorize image filters code

a213ef3: file chooser dialogs: rework image filters code

7429e7e: file chooser dialogs: rework archive filters code

7bfa995: file chooser dialogs: tweak archive/image filters

ace7641: thumbnail tools: allow disabling archive support

99d57ea: image handler: improve get_thumbnail

0621695: edit dialog: improve thumbnail creation

dbe3043: event: minor simplification

e3df8f3: run: fix logger init + add debug/info levels

a63e47d: main: fix draw_image prototype

74904ed: main: fix scrolling position on page change

d17a24e: test/image tools: add implied rotation test

29ed6fe: image tools: tweak get_implied_rotation documentation

8ca6549: test/image tools: update rotation test

7d04261: image tools: fix get_implied_rotation for PNG files

67842e6: image tools: factorize fit_in_rectangle/fit_pixbuf_to_rectangle

7184036: image handler: improve virtual double page detection

b12fd70: status bar: fix visibility after initialization

bbc0655: ui: remove reference to file_load_failed

4c46eed: file manager: get rid of file_load_failed

b125ce8: file handler: make sure file_opened is called

c7258ab: fix toolbar buttons sensitivities

fe51370: thumbbar: don't hide on clear

b0de089: main: rework handling of "toggle" widgets

af657ea: file handler: move (most) UI code out to main

f0a06b8: main: improve info panel

231f737: worker thread: change semantics for dealing with unique orders

f8624b2: thumbnail view: rework API

c4b01d8: thumbbar: update to new thumbnail view API

7624cd6: library: update book area to new thumbnail view API

e393e8d: edit dialog: update to new thumbnail view API

5d4feb9: thumbbar: fix active page display

da99567: thumbbar: simplify layout size handling

65fe44a: thumbbar: small cleanup

921891d: thumbbar: cleanup

895938b: thumbbar: align selection on change

5344ab6: fix "don't act on click to focus" handling

72f1c49: library: cleanup book area code

ee467f5: library: avoid annoying 'row resizing' effect in book area

5a6a324: library: use tighter layout for book area

13e2b5e: messages: update translations

d098c6c: messages: update french translation

49f5cf7: image tools: re-enable loading images with PIL on Windows

ec26137: tools: simplify number_of_digits

e7775c5: main: fix rotation handling

1ca0c89: portability: remove py2exe workaround

81d9a35: add wine-helper.sh script

ac8683a: rundown.py

4cdf28d: gettext.sh

ad624d0: archlinux: add directory

8a260aa: [wip] main: handle possible segfault on resize

dfa5fc3: [wip] archive: fix tar unicode support

a111d0d: [wip] drop py2exe support

685b1f6: [wip] win/install.txt

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

a4f8c38: [wip] comicthumb: archive.ask_for_password

This way I can push up until the point you're okay with.

The changes compared the previous state are:

file handler: remove _must_call_draw hack

Not needed, as main.set_page() handles the redraw if needed.

file chooser dialogs: tweak archive/image filters

Only enable archive/image filters when applicable.

thumbnail tools: disable archive support when not applicable

thumbnail tools: allow disabling archive support

image handler: improve get_thumbnail

edit dialog: improve thumbnail creation

This fix the issue you mentioned with a "null" image file.

run: fix logger init + add debug/info levels

Initialize logger earlier, so debug/info levels can be used when
importing some modules.

event: minor simplification

fix scrolling position on page change

main: fix draw_image prototype

main: fix scrolling position on page change

Fix this issue:

When flipping to the previous page, and it's not available, then the
scroll position will be set to the start when it's finally drawn,
instead of the end of the page.

rework the UI code for handling the "toggle" widgets

I renamed some stuff to make the code more comprehensible, and fixed the previously mentioned issue:

When opening an empty archive, the OSD warning message can be scrolled
horizontally.

image tools: re-enable loading images with PIL on Windows

remove the preference, unconditionally enable PIL use on Windows

refactor the code as per your request

fix the issue you mentioned: list instead of tuple for BMP format

fix load_pixbuf_size alpha handling when PIL is used

fix number_of_digits as per your comments

Related

Feature Requests: #92
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-09

Sorry to keep you waiting.

2010 is ancient; admittedly the last release for PyGtk is only from 2012.

Also, I looked at the code for MComix-1.01, and icons are not loaded by PIL: image_tools.load_pixbuf_data is used, and its implementation uses GDK.
[…]
To be clear: this is not about replacing some library for the other; the code still expects a GDK pixbuf after loading, and still switches to PIL for some operations. This is only about which one is used for decoding.
[…]
It's not about image loading being slow, rather it's about the possibility of making it faster.

I think I understand what do you want to achieve, but I think that faster decoding simply is not enough. If you want to improve the performance of the application, you need to consider the "rendering pipeline" as a whole, from loading a file up to rendering pixels on the screen, including all the features and environmental issues. It does not help much if decoding is done fast if the final rendering turns out to be terribly slow.

Of course, it might be the case that everything you do always improves performance, and I really like to use "optimized" software. However, I'm simply not yet convinced that it will turn out as expected. We need to address this issue, I am totally aware of that. But we should do so for a later release and not now. (If you don't know what to do else, create a ticket for it and make a table that lists all visible and invisible features of MComix involved in image processing and, for each of them, states which libraries can implement this feature and which library is currently used for this feature. By "features", I mean "render on a widget", "decode", "only decode header", "caching", "magnifying lens", "calculate histogram", "affine transforms", "sharpening", "dynamic background color", "thumbnail creation" and so on.)

The GTK3 transition does not change the architecture: 90% of it was just renaming stuff.

I see.

I was thinking the same thing:

add an "All supported formats" entry, selected by default, since it's the best mode for 99% of the use cases

find a way to make the list clearer, I don't know if adding a separator is possible, we'll look into it (I tried to see if bolding an entry is possible: it's not)

If it turns out to be highly complicated, a more useful ordering should be enough here.

Except when applying a mirror tranformation when an automatic rotation is already active, then it's the other way round: "rotation first, mirroring last" is more intuitive.

You have a point there.

Sure, I'm all in favor of simplifying the implementation, but we need to figure the specification first.

What kind of specification are you talking about?

In the mean time, I reordered the commits in my branch to move the more "controversial" stuff toward the end (you can ignore the last 9 changes), and made a few more changes.

Here is a list of the commits in the right order (since Github does not handle rebase very well: commits are reordered chronologically):

[…]

This way I can push up until the point you're okay with.

Good idea. I need some time to review the changes since there are a lot of them. (I'm sorry to keep you waiting again.)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-10

Okay, here you are, some comments: I like how you reduce dependencies (especially circular ones) and simplify code.

Changes that might need some improvement:

@a0f5a7cc6c6dc6c81675e9e84c62343337e7382b: Please extract file size formatting to a function in tools.py that maps an integer value to an appropriate string, e.g. 12899 → "12.6 KiB".

@ec261376ae9dc52e018dc4ed42f658d55f7a8c16: If n == 0, it should return 1. (Zero is the only number where you need leading zeroes.)

Changes I don't understand what they are for or why they are necessary:

ace7641e94510c0654f5a8a14935dcaf1218d3ad

231f73794ec3f549652e60828c460a1476bbbd85

Some bugs I just ran into (tested at a4f8c38921190903eb6c31dd7097257273ca52aa):

Open a book with many pages. Browse through the pages using the keyboard (e.g. PgDown) until the thumbnailer shows pages far away from the beginning. Select File → Refresh. Instead of moving the viewport to the currently selected image, the thumbnailer displays the beginning of the book. More often than not, the thumbnails of these images will not be rendered (which is actually fine in this case since they should not be visible in the thumbnailer in the first place). Hovering the mouse cursor over them makes them appear one after another. No exception is thrown. It seems that this happens every time the current image has not been reached by a direct click on the respective thumbnail. Pressing PgDown or even Refresh, for example, are not "direct" enough.

Some thumbnails are scaled without keeping aspect ratio. It seems that only those images images are affected which have an alpha channel and, at the same time, need to be rescaled to fit. An example is mcomix/images/mcomix-large.png.

Race condition: If you keep F (i.e. your "toggle fullscreen" key) pressed, you might end up with widgets visible in fullscreen mode.

For this bug to reproduce, extract the attachment to /tmp and copy it so you get dirs like test1, test2, test3, etc. Open one of them in MComix so you can switch to the other dirs by pressing PgDown and PgUp. Note that if you only use PgDown, everything works fine. However, if you use PgUp, the last NUL file of the dir you wanted to switch to is actually identified as an archive, resulting in "No images in 'test_nul.png'". If you press PgUp again, the dir that contains this NUL file is skipped (since the dir had been identified as an archive) but the next dir works fine (i.e. you'll skip every second dir this way). Note: This bug is already present in the current SVN revision.

Not really a bug, but if you've opened a directory in MComix, the File → Properties dialog shows virtually nothing in the "Archive" tab. This makes it look somewhat broken. Ideally, this tabs shows information similar to what it shows when you've opened an archive.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-10

Sure, I'm all in favor of simplifying the implementation, but we need to figure the specification first.

What kind of specification are you talking about?

How MComix should handle transformations.

Changes that might need some improvement:

-@a0f5a7cc6c6dc6c81675e9e84c62343337e7382b: Please extract file size formatting to a function in tools.py that maps an integer value to an appropriate string, e.g. 12899 → "12.6 KiB".

Please create a bug to track this for later.

-@ec261376ae9dc52e018dc4ed42f658d55f7a8c16: If n == 0, it should return 1. (Zero is the only number where you need leading zeroes.)

Damn...

Changes I don't understand what they are for or why they are necessary:

ace7641e94510c0654f5a8a14935dcaf1218d3ad

This is for the fix to the thumbnail issue: no broken image for a file containing only NUL bytes. Plus there is really no reason for the thumbnailer to check if each file is an archive when we're only feeding it images.

231f73794ec3f549652e60828c460a1476bbbd85

Needed for the following thumbnail code rework. The thumbnail orders are (uid, iter) tuples, but only the uid identify the order uniquely.

Some bugs I just ran into (tested at a4f8c38921190903eb6c31dd7097257273ca52aa):

Open a book with many pages. Browse through the pages using the keyboard (e.g. PgDown) until the thumbnailer shows pages far away from the beginning. Select File → Refresh. Instead of moving the viewport to the currently selected image, the thumbnailer displays the beginning of the book. More often than not, the thumbnails of these images will not be rendered (which is actually fine in this case since they should not be visible in the thumbnailer in the first place). Hovering the mouse cursor over them makes them appear one after another. No exception is thrown. It seems that this happens every time the current image has not been reached by a direct click on the respective thumbnail. Pressing PgDown or even Refresh, for example, are not "direct" enough.

I cannot reproduce this.

Some thumbnails are scaled without keeping aspect ratio. It seems that only those images images are affected which have an alpha channel and, at the same time, need to be rescaled to fit. An example is mcomix/images/mcomix-large.png.

Already fixed. I'm in the process of adding more tests to test/test_image_tools.py.

Race condition: If you keep F (i.e. your "toggle fullscreen" key) pressed, you might end up with widgets visible in fullscreen mode.

I can't reproduce this on a4f8c38921190903eb6c31dd7097257273ca52aa, or Subversion. I can reproduce it with MComix 1.01; including the fact that the page may not be correctly rendered (e.g. adjust to width not taking into account the new fullscreen width, and a variation: the widgets stay hidden when ending up in windowed mode).

For this bug to reproduce, extract the attachment to /tmp and copy it so you get dirs like test1, test2, test3, etc. Open one of them in MComix so you can switch to the other dirs by pressing PgDown and PgUp. Note that if you only use PgDown, everything works fine. However, if you use PgUp, the last NUL file of the dir you wanted to switch to is actually identified as an archive, resulting in "No images in 'test_nul.png'". If you press PgUp again, the dir that contains this NUL file is skipped (since the dir had been identified as an archive) but the next dir works fine (i.e. you'll skip every second dir this way). Note: This bug is already present in the current SVN revision.

Looks like the same thing as the thumbnail "missing broken image" issue: testing for an archive type is performed first. In this case tarfile.is_tarfile will return True, hence the problem.

Not really a bug, but if you've opened a directory in MComix, the File → Properties dialog shows virtually nothing in the "Archive" tab. This makes it look somewhat broken. Ideally, this tabs shows information similar to what it shows when you've opened an archive.

Let's create a bug to track this for later.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-11

What kind of specification are you talking about?

How MComix should handle transformations.

I see. (I wonder whether the table I mentioned above might help us making a decision, too.)

-@a0f5a7cc6c6dc6c81675e9e84c62343337e7382b: Please extract file size formatting to a function in tools.py that maps an integer value to an appropriate string, e.g. 12899 → "12.6 KiB".

Please create a bug to track this for later.

Understood.

ace7641e94510c0654f5a8a14935dcaf1218d3ad

This is for the fix to the thumbnail issue: no broken image for a file containing only NUL bytes. Plus there is really no reason for the thumbnailer to check if each file is an archive when we're only feeding it images.

I see. Minor improvement: Please make the archive_support flag immutable, i.e. remove set_archive_support and force all callers to set this flag on object creation. This might prevent bugs in the future.

231f73794ec3f549652e60828c460a1476bbbd85

Needed for the following thumbnail code rework. The thumbnail orders are (uid, iter) tuples, but only the uid identify the order uniquely.

I see.

I cannot reproduce this.

I'll try to either fix this myself or find the reason for why you have trouble reproducing it.

Some thumbnails are scaled without keeping aspect ratio. It seems that only those images images are affected which have an alpha channel and, at the same time, need to be rescaled to fit. An example is mcomix/images/mcomix-large.png.

Already fixed. I'm in the process of adding more tests to test/test_image_tools.py.

Okay.

Race condition: If you keep F (i.e. your "toggle fullscreen" key) pressed, you might end up with widgets visible in fullscreen mode.

I can't reproduce this on a4f8c38921190903eb6c31dd7097257273ca52aa, or Subversion. I can reproduce it with MComix 1.01; including the fact that the page may not be correctly rendered (e.g. adjust to width not taking into account the new fullscreen width, and a variation: the widgets stay hidden when ending up in windowed mode).

If you manage to fix it for MComix 1.01, please include the fix in your branch so I can test it with my set up.

Looks like the same thing as the thumbnail "missing broken image" issue: testing for an archive type is performed first. In this case tarfile.is_tarfile will return True, hence the problem.

I think this is related to a behavior of MComix I never understood: If you start at somedirA/archive.7z and go to the next book via PgDown but there is no other archive in somedirA next to the current one, you'll be taken to somedirB which is next to somedirA and has not have anything to do with the previous "shelf". If you go back using PgUp, you will see the images in somedirA (something that was not possible when you were walking over the archives in it!) but you will not get to the archives in somedirA. That is, MComix changed the interpretation of somedirA from being a shelf (containing archive books) to a book (containing plain images).

In other words, I don't understand why MComix tries to identify archives there in the first place. The test* dirs should be treated as books and MComix should not apply recursion on them (since these books are dirs and not archives). At least that's what I think MComix should behave. What do you think? (I guess this needs another ticket as well.)

Not really a bug, but if you've opened a directory in MComix, the File → Properties dialog shows virtually nothing in the "Archive" tab. This makes it look somewhat broken. Ideally, this tabs shows information similar to what it shows when you've opened an archive.

Let's create a bug to track this for later.

Understood.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Benoit Pierre - 2015-04-11
  
  This is for the fix to the thumbnail issue: no broken image for a file containing only NUL bytes. Plus there is really no reason for the thumbnailer to check if each file is an archive when we're only feeding it images.
  
  I see. Minor improvement: Please make the archive_support flag immutable, i.e. remove set_archive_support and force all callers to set this flag on object creation. This might prevent bugs in the future.
  
  This would make it inconsistent with the way other flags work (like force_recreation, or store_on_disk). How about changing the default to False instead? This way archive support has to be explicitely enabled.
  
  Race condition: If you keep F (i.e. your "toggle fullscreen" key) pressed, you might end up with widgets visible in fullscreen mode.
  
  I can't reproduce this on a4f8c38921190903eb6c31dd7097257273ca52aa, or Subversion. I can reproduce it with MComix 1.01; including the fact that the page may not be correctly rendered (e.g. adjust to width not taking into account the new fullscreen width, and a variation: the widgets stay hidden when ending up in windowed mode).
  
  If you manage to fix it for MComix 1.01, please include the fix in your branch so I can test it with my set up.
  
  Maybe a different window manager? Is it worth investigating / fixing? I've never had any problem switching in/out of fullscreen, so if the problem only happen when hammering the 'F' key, then it's not a big issue IMHO. It may also be a Linux only issue; as fullscreen support is complicated on Linux (X11 + window mangager + widget library...). I don't know if it behaves the same way on a real Windows machine, but the fullscreen transition when using Wine (with a simulated desktop) is clean: no ugly intermediate state.
  
  Looks like the same thing as the thumbnail "missing broken image" issue: testing for an archive type is performed first. In this case tarfile.is_tarfile will return True, hence the problem.
  
  I think this is related to a behavior of MComix I never understood: If you start at somedirA/archive.7z and go to the next book via PgDown but there is no other archive in somedirA next to the current one, you'll be taken to somedirB which is next to somedirA and has not have anything to do with the previous "shelf". If you go back using PgUp, you will see the images in somedirA (something that was not possible when you were walking over the archives in it!) but you will not get to the archives in somedirA. That is, MComix changed the interpretation of somedirA from being a shelf (containing archive books) to a book (containing plain images).
  
  In other words, I don't understand why MComix tries to identify archives there in the first place. The test* dirs should be treated as books and MComix should not apply recursion on them (since these books are dirs and not archives). At least that's what I think MComix should behave. What do you think? (I guess this needs another ticket as well.)
  
  I know I find the current behaviour with directories vs. archives handling confusing and complicated. My fist instinct would have been to handle a directory like an archive. But then there is the recursive behaviour, which might not be ideal in this case, especially if the tree contains archives.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Ark - 2015-04-13
    
    For the record: I created [bugs:#83] for the "directories vs. archives handling" issue.
    
    Related
    
    Bugs: #83
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-13

I see. Minor improvement: Please make the archive_support flag immutable, i.e. remove set_archive_support and force all callers to set this flag on object creation. This might prevent bugs in the future.

This would make it inconsistent with the way other flags work (like force_recreation, or store_on_disk). How about changing the default to False instead? This way archive support has to be explicitely enabled.

I think that changing the default does not improve much. I'd prefer it the other way around: Eliminate the setters you mentioned. It seems that both set_force_recreation and set_store_on_disk are only called directly after object creation. Thus, it should be easy to move the flag setting to the constructor and remove the setters instead. Less state → more functional → fewer bugs.

If you manage to fix it for MComix 1.01, please include the fix in your branch so I can test it with my set up.

Maybe a different window manager? Is it worth investigating / fixing? I've never had any problem switching in/out of fullscreen, so if the problem only happen when hammering the 'F' key, then it's not a big issue IMHO.

Well, yes and no. Fixing would be nice since it's not really "hammering" you need for this to happen. (4 keystrokes per second are sufficient in my case.) Also, it does not happen with MComix 1.01. On the other hand, it's not that problematic. I'm investigating.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-13

Concerning this fullscreen issue: git bisect identified b0de0895b4d0d87150242b70c6e34ff9a1138426 as the culprit. Unfortunately, this commit is quite big. I just played around with it a little. Adding one or both of the following lines to the end of change_fullscreen in main.py does not fix it, it only make it less likely to happen:

self._update_toggles_visibility() self.draw_image()

Also, resizing the window forces the widgets to be re-laid out correctly, both if they are not visible when they should be (after switching to windowed mode) and if they are visible when they should not be (after switching to fullscreen).

I'm further investigating. If you have any ideas concerning this issue, I'd be glad to read them.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-13

Another bug I just found: While in double page mode, the window title bar and all widgets except the main viewport always behave as if there are currently two images visible, even if there should be only one image opened. As already said, only the main viewport behaves correctly.

git bisect identified af657ea1266b8f0dfe9a3333a1fd8c402b509658 as the first bad commit in this case.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-13

Concerning this fullscreen issue: git bisect identified b0de0895b4d0d87150242b70c6e34ff9a1138426 as the culprit. Unfortunately, this commit is quite big. I just played around with it a little. Adding one or both of the following lines to the end of change_fullscreen in main.py does not fix it, it only make it less likely to happen:

self._update_toggles_visibility()
self.draw_image()

Yes, not the right fix, as the GTK documentation states:

The fullscreen() method requests the window manager to place the window in the fullscreen state. Note you shouldn't assume the window is definitely full screen afterward, because other entities (e.g. the user or window manager) could unfullscreen it again, and not all window managers honor requests to fullscreen windows. But normally the window will end up fullscreen. Just don't write code that crashes if not.

So I would leave this part of the code as is.

Also, resizing the window forces the widgets to be re-laid out correctly, both if they are not visible when they should be (after switching to windowed mode) and if they are visible when they should not be (after switching to fullscreen).

I'm further investigating. If you have any ideas concerning this issue, I'd be glad to read them.

resize_event uses is_fullscreen, instead of looking at the real window state (get_state() & gtk.gdk.WINDOW_STATE_FULLSCREEN), so maybe that's the issue

if that does not fix it, I would register to the window-state-event signal to monitor the fullscreen change, and add the call to draw here

maybe get rid of is_fullscreen altogether? (or change it to a property that check get_state)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Benoit Pierre - 2015-04-15

Ok, so I've pushed some more changes:

fixup! tools: simplify number_of_digits

fixup! file chooser dialogs: tweak archive/image filters

fixup! file handler: move (most) UI code out to main

This fix the virtual double page information in the status bar.

add more tests for the image tools module, and fix the associated bugs discovered

test: fix 01-JPG-Indexed.jpg and 03-PNG-RGB.png mode

test: add pytest based testsuite runner

test: support testing another MComix version

test: add custom testcase class

test/image tools: add more tests

fixup! image tools: re-enable loading images with PIL on Windows

image tools: fix fit_in_rectangle handling of scaling quality

image tools: factorize fit_in_rectangle

image tools: improve load_pixbuf_size

image tools: minor cleanups

image tools: fix pil_to_pixbuf

image tools: fix fit_in_rectangle

image tools: fix load_pixbuf_size

wine helper: simplify test command

Use new testsuite runner.

wine helper: install fixed mimetypes module on setup

wine helper: update distribution documentation

add Python licence, news, and readme files

add PKG-INFO.txt for czipfile and Pillow

simplify thumbnail API

Remove the various setters that change the thumbnailer behaviour:
move the flags to the constructor.

main: fix horizontal/vertical flip handling on double pages

Flip the whole virtual page, instead of flipping individual pages.

I think the way transformations are handled at this point is good enough, the final behaviour is similiar to MComix-1.00 (rotations first, flipping last), with a few differences:

Exif rotations are applied to individual images first

other rotations are done on the whole (virtual) page (so double page mode is supported)

same for horizontal/vertical flip

you can further rotate the page after an automatic rotation

I think that having the horizontal/vertical flip being done last makes sense: horizontal flips the page left/righ, while vertical flips up/down. And again, this is the same behaviour as MComix 1.00, extended to support double page mode.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ark - 2015-04-30

Sorry to keep you waiting. The current bpierre branch (up to e56439073feade9d606e33ae04a81e28d9b87d16) looks promising. It seems to fix the fullscreen issue I mentioned earlier along with many issues discussed in other threads. Thank you very much.

I have to admit that, though, that it's quite difficult to keep track of the changes thanks to history rewrites. But maybe I'm simply using the wrong tools. May I ask you which tool(s) you are using? Plain Git CLI or some more sophisticated wrapper/GUI/whatever?

About transformations: Okay, let us keep it this way. It still feels a bit strange for me if I seemingly rotate everything the other way around if there is some kind of flipping involved, but it does not look buggy anymore.

Some bugs that are still present:

The names of the files are not reversed in manga mode. This applies to both the status bar as well as the window title.

You can run into empty PNG files and MComix still treats them as archives.

Maybe we should apply the transformations to the smart scrolling directions as well?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

new batch of changes for inclusion

GTK+ comic book viewer.

Group

Searches

Help

#41 new batch of changes for inclusion

Related

Discussion

Related

Related

Related