refactoring in pageview class

[Jonathan Schultz]

Hear hear!

PageView really is a a mess, I presume because it has been repeatedly 
added to as okular has grown from kpdf and never been subject to a 
proper clean-up. Notably (and in addition to the 
PageView/PageViewPrivate confusion you mention), it has massively long 
functions (eg mouseReleaseEvent has over 700 lines), overly complex and 
sometimes redundant internal data structures, and its logic is quite 
opaque. Not surprisingly, there are inconsistencies and bugs in the UI - 
here are a couple I have raised to little or no feedback:

https://bugs.kde.org/show_bug.cgi?id=363776
https://bugs.kde.org/show_bug.cgi?id=361538
https://git.reviewboard.kde.org/r/127496/

My personal interest stems from having forked okular for my own project 
(https://github.com/jschultz/okular-tagging), for which I am extending 
the UI. Given the apparent lack of enthusiasm for small but helpful 
changes (such as your patch to use escape to leave full-screen mode, 
like just about every full-screen application in the world!) I have 
stopped worrying about this situation and just forged ahead with my own 
code.

Frankly I think pageview would benefit from a complete refactor/rewrite. 
I see that KDE has announced a shiny new UI toolkit called Kirigami - 
maybe that's the opportunity to re-write Okular's (rather clunky IMHO) UI.

Best,
Jonathan


On 14/02/17 07:13, habruening wrote:
> Hi Okular developers!
>
> I am trying to start with KDE development. It is my first open source
> initiative. I think, Okular is an interesting point to start. My first
> patch (https://git.reviewboard.kde.org/r/129773/) was not accepted.
> Anyway, I carry on.
>
> I think, I could try some code cleanup work. So please let me know if
> the following idea is welcome.
>
> When I see pageview.cpp, there is an object PageViewPrivate *d.
> Obviously it is PIMPL. But I cannot see a private implementation.
> Everything is done in PageView. PageViewPrivate is just a data class. I
> would like to make PageViewPrivate really a private implementation. I
> don't know yet how far I can separate PageView and PageViewPrivate. But
> I think they could have individual responsibles. One is the interface to
> the outside world and the other is all the implementation details.
> Following the single-responsibility-principle, I cannot believe that
> pageview.cpp must have 5000 lines of code.
>
> But one question: Is there a technical reason for hiding all the data in
> a d-pointer? The code looks as if the author did not want to use a
> d-pointer but was forced to it.
>
> Best regards!
>