refactoring in pageview class

[Albert Astals Cid]

El dilluns, 13 de febrer de 2017, a les 21:13:55 CET, habruening va escriure:
> 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.

Personally I'm not very happy about refactorings, usually they just end up 
either:
 * Causing regressions because there's a corner case the refactoring didn't 
think of
 * Being as complex as the original code, because original code is complex for 
a reason.

But I'm not Okular's maintainer anymore so you just need to find someone that 
agrees to review and commit your code.

Also *personally* I think time would be better spent fixing some actual bugs 
instead of just making the code nicer to look at, but it is your time, so you 
spend it as you think it's better/more fun for you :)

> But one question: Is there a technical reason for hiding all the data in
> a d-pointer? 

The header says 
   // don't want to expose classes in here

So I guess saves a few forward declaration/includes and as always makes 
keeping binary compability easier (even though pageview.h is not installed so 
that's a smaller concern).

Cheers,
  Albert


> The code looks as if the author did not want to use a
> d-pointer but was forced to it.
> 
> Best regards!