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!
