#63 MComix might need a rewrite

[Ark]

Hi oddegamra

I made several attempts to find abstractions that make it easy to implement the requested features or to fix bugs. However, every attempt ended in doing nothing but wonder how complex event.py, main.py and image_handler.py currently are.

So I tried to simplify these source files first. But then I realized that they are way to powerful to do so. (At least that is what I think.) Then I tried to make use of the code in scrolling.py, but I realized again that simply adding the appropriate code to event.py would make said source file even more powerful.

Well, you see, the current source code does not look carefully designed to me. (Trivial names like main.py are testaments to this issue.) That is, I think about a (complete?) rewrite of MComix.

I'm currently thinking about how the new design could look like. The downside is that this implies a lot of work. What do you think about this approach in general? Any ideas, requests or suggestions?

[Oddegamra]

Well, there would be a number of things I would want, for example:

• Better separation of GUI and model parts of the application. Main.py, as you mentioned, is the main suspect. When I started on MComix, I tried to write more test cases for common problems I encountered, but came to realize after a while that unit tests are pretty damn hard to do when you have to simulate UI interaction to get to the part you want to test.
• A purely threaded, worker/queue model for archive extraction and image loading. When I re-wrote the archive code, I tried to remove all blocking calls to various FileHandler methods, but some still remain and are quite difficult to get rid off, which sometimes leads to deadlock situations, for example, when an archive is corrupted.
• A better model for drawing images in the main display area, as hinted around here in various tickets.

There would probably be other nice things that could be done. Well, in theory, at least. As it stands, MComix does a lot of things in the background that aren't immediately obvious when using the GUI, so naturally there will be a lot of code to support these actions. As it happens, most of it is centered around the main functionality of MComix, i.e. main.py and image_handler.py. The problem, I think, is that a common abstraction is missing in many methods, so they have a lot of if/else branches and duplicated code here and there. To tell the truth, I occasionally tried to come up with ideas to simplify the code, such as introducing additional helper classes or refactor the code, but wasn't successful that often.

By the way, did you know that Comix has a history of two rewrites to this day? One by herrekberg from Comix 3 to 4, and another from Comix 4.0.5 to MComix 0.9.x by oxaric (granted, this wasn't as extensive). So, at least twice two different people tried their best to simplify and improve the code. I don't know the Comix 3 codebase, so I can't really tell how much better the situation is now, but since there is obviously still room for improvement, I come to the conclusion that most likely, a number of old problems have been fixed, yet a number of new problems have been introduced. This is the main problem of rewriting everything. For one, you most likely will (unknowingly) remove a number of features, introduce new bugs, and additionally, will spend a large amount of time on getting things back in working condition that no one (except for you and me) will appreciate.

Personally, I think it would be better to tackle one problem after another in small, manageable steps. For example, simplifying the drawing code would be a good start. This is a localized change that doesn't touch that many places, is easily testable and would probably bring immediate benefits. If this doesn't work without touching something else first, well, why not do that first without actually touching any drawing code? I believe such an approach will likely have more chances of success.

So, as a first step, it would be great if you could post what exactly you want to accomplish/implement, and what is stopping you from doing so. Then we can move on from there.

[Ark]

So, as a first step, it would be great if you could post what exactly you want to accomplish/implement, and what is stopping you from doing so. Then we can move on from there.

Despite the fact that my demo patch looks extremly ugly (at least to me), I would like to (and should) integrate scrolling.py, even if I think that doing so will make the code look even worse. (This is merely a note to myself.)

I also thought about issues like feature [#76], feature [#74], bug [bugs:#60] and your double page mode rant (see https://sourceforge.net/p/mcomix/feature-requests/75/#0043), and I came to the conclusion that solving these issues would be way easier in a redesigned architecture than in the current one.

To tell the truth, I occasionally tried to come up with ideas to simplify the code, such as introducing additional helper classes or refactor the code, but wasn't successful that often.

I know exactly, what you mean. Well, because of that, I think it would be easier to rewrite from almost-scratch. That is, I would like to implement a new architecture by copying existing parts in a way that they fit into the new design.

By the way, did you know that Comix has a history of two rewrites to this day? […] I come to the conclusion that most likely, a number of old problems have been fixed, yet a number of new problems have been introduced. This is the main problem of rewriting everything. For one, you most likely will (unknowingly) remove a number of features, introduce new bugs, and additionally, will spend a large amount of time on getting things back in working condition that no one (except for you and me) will appreciate.

No, I didn't know about previous rewrites, but thank you for telling me about this. However, I know that rewriting is a risky task. Because of that, I would like to use a new directory (mcomix2 or something) where I (or we) can write the new code next to the productive one. (I don't know whether creating a new branch for this might be the better idea. However, I'm not used to branches in SVN.)

The problem, I think, is that a common abstraction is missing in many methods, so they have a lot of if/else branches and duplicated code here and there.

That is one of the major problem, indeed. Example: Do you remember what zoom.py did look like in [r819]? The _scale_up variable was evaluated by each FitMode independently. But in fact, allowing automatic upscaling is orthogonal to all FitModes. Therefore, I extracted _scale_up so it will be respected in every FitMode, while the FitModes themselves do not even need to know about _scale_up anymore. Actually, this change could be implemented only because zoom.py has a sharply defined purpose and acts (almost) independently from the rest of the program. I hope that all features of MComix can be designed and implemented this way. This would also solve the issues you and I just mentioned (also about unit tests and everything).

I'm currently elaborating the new architecture I have in mind, and it already looks very promising to me. (Admittedly, it also looks like a lot of work.) I will post it here so we can discuss this afterwards, if you want to.

[Ark]

Sorry to keep you waiting. I'm quite busy right now.

Here you have a brief description of the main characteristics of some objects/modules/classes that came to my mind. Note that this list is not complete; I don't have much time so I thought that it would be better to post a half-baked draft now than a perfect one never. (Actually, this draft is almost two weeks old. I'm sorry.)

---

Data

• A Location is a sequence of paths or URIs to locate things like Images or Books (see below). A valid Location might roughly look like ["http://example.com/path/to/outer/archive.7z", "/path/to/inner/archive.zip", "/path/to/comic", "actualimage.png"] with (hopefully) obvious semantics.
• A Page is a part of a Book. A Page has a name, dimensions (as in width and height), and a Location that describes where the actual image is located.
• A Book has a title, a Location and a list of Pages. Note that the Pages of a book do not need to appear in any particular order. However, using the decorator pattern, Books can be "stacked" to reflect any kind of page ordering or filtering, for example.

Basic actions

• A Reader navigates through the sequence of pages of a Book. Actions like "flip page" or "flip protection" are implemented here.
• A Scroll arranges rectangles (for example, the Pages of a Book) in an at least two-dimensional way. Note that this covers single page mode, double page mode and continous layout, and the actual arrangement is influenced by manga mode and western mode. Furthermore, a Scroll does not know about scaling, rotation or whatever might influence the dimensions of the rectangles. Instead, all affine transforms need to be applied beforehand.
• A Viewport represents a sliding window (that is, a rectangle) for a Scroll. This window can be moved around by scrolling (which is influenced by western mode or manga mode, for example). The main purpose of a Viewport is to calculate its intersection with another rectangle (for example, a Scroll). Note that, unlike a GtkViewport, this Viewport is not bound to any specific widget/toolkit/whatever framework but is a completely GUI independent object to, for example, determine the pages the human reader is currently looking at. However, the state of such Viewport can be easily mapped to actual GUI viewports and vice versa. Note that this abstraction can be used to facilitate implementing feature [#74] and that it might help fixing bug [bugs:#60].
• A Slideshow controls Viewports and/or Readers by calling appropriate methods concurrently. Note that this feature makes it necessary to synchronize calls to Viewport and/or Reader objects.

Image Rendering

• An ImageFilter applies a kernel to an image. This can be used for sharpening, for example. Applying this before scaling is performed solves an issue mentioned in feature [#70].
• Scaling, rotation and mirroring are described as AffineTransforms that result in a matrix which describes the final transform to be applied.
• An ImageRenderer reads a portion of an Image and uses an InterpolationMethod (such as bilinear interpolation) and an AffineTransform to render a portion of a destination Image.
• A BackgroundRenderer renders the portions of an images that are not covered by the rectangles of a Scroll.
• A ViewportRenderer combines an ImageRenderer, a BackgroundRenderer and a Viewport to draw the image that will actually be seen in the main window at the end.
• A ColorCurve describes the way a given color is mapped to another color similar to http://docs.gimp.org/en/gimp-tool-curves.html. This can be used, for example, to adjust contrast, brightness and gamma. Appling a ColorCurve to the image a ViewportRenderer just have rendered will implicitly adjust the background color as well.

---

Sorry, image loading and caching are not part of this draft. I hope that I can make up an appropriate design for this issue soon.

Feel free to comment the current draft. To be honest, I don't know whether there are existing implementations for the things I described, so it would be very helpful if you can refer to them.

[Ark]

FYI: I'm still alive, really. ;) However, free time is rare, sorry.

I'm currently working on Reader, Scroll and Viewport. Sorry, no files yet.