Re: Review Request 130171: Fix page-jumping-back-issue commonly encountered when in presentation mode
> On jul. 3, 2017, 9:07 p.m., Albert Astals Cid wrote: > > Sorry but no, you can't fix presentation mode by removing code out of > > pageview that you claim you don't know what it does. Debug more and come up > > with a better patch. > > Alexander Schlarb wrote: > Well, can you tell me what that code I'm trying to remove ought to do? > (Even a hunch would be super useful!) Read the code of PageView::slotRequestVisiblePixmaps maybe? - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130171/#review103404 --- On jul. 3, 2017, 8:54 p.m., Alexander Schlarb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130171/ > --- > > (Updated jul. 3, 2017, 8:54 p.m.) > > > Review request for Okular. > > > Repository: okular > > > Description > --- > > This fixes an issue I've commonly encountered while quickly working myself > through large PDFs with many pages (such as > [this](https://storage.ninetailed.ninja/index.php/s/FS58uge8yKCvjlB)). > > Steps to reproduce the issue: > > 1. Open Okular > 2. For easily reproducing the issue change *Settings* ? *Performance* ? > *Memory Usage* to *Greedy* (the issue can be reproduced without it, but it's > harder an alot more random) > 3. Open a PDF with many pages > 4. Immidiately go to presentation mode (*Ctrl*+*Shift*+*P*) > 5. Click on the screen or press the left-arrow keyboard button > > Expected behaviour: > > * The next slide is shown > > Actual behaviour: > > * The next slide *is* indeed shown for a split second before the one is sent > back to the first slide (this keeps being the case until all slides have been > preloaded, then it stops) > * Interestingly enough (and I frankly have no idea why) everything works > normally if one quickly double-clicks to skip forward by two slides, slides > can be viewed normally after that as long as you don't go back to the second > slide again…? > > > This patch fixes the problematic behaviour by removing code that looks its a > bogus fixi from something that shouldn't be a problem… maybe. > Unfortunately the one attached comment isn't helpful at all in describing > what this check was originally supposed to do – just like the commit message > for that change (obtained through `git blame`): > > commit d2d2fa3b2a4fa8f4b554b87ec13ee5a15b64f35d > Author: Mailson Menezes > Date: Sun Nov 11 23:09:12 2012 -0300 > > Refactoring PageView > > I therefor can only hope that this solution doesn't break anything. Guidance > of somebody with in-depth knowledge of the code-base would be apprechiated on > this. :-) > > > Diffs > - > > ui/pageview.cpp acacfb90 > > Diff: https://git.reviewboard.kde.org/r/130171/diff/ > > > Testing > --- > > Loaded a few different PDFs that had previously exibited the described > behaviour and verfied that they are fixed now. Also scrolled through them in > non-presentation mode, to verify that nothing else appears to be broken now. > > > Thanks, > > Alexander Schlarb > >
Re: Review Request 130171: Fix page-jumping-back-issue commonly encountered when in presentation mode
> On Juli 3, 2017, 11:07 nachm., Albert Astals Cid wrote: > > Sorry but no, you can't fix presentation mode by removing code out of > > pageview that you claim you don't know what it does. Debug more and come up > > with a better patch. Well, can you tell me what that code I'm trying to remove ought to do? (Even a hunch would be super useful!) - Alexander --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130171/#review103404 --- On Juli 3, 2017, 10:54 nachm., Alexander Schlarb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130171/ > --- > > (Updated Juli 3, 2017, 10:54 nachm.) > > > Review request for Okular. > > > Repository: okular > > > Description > --- > > This fixes an issue I've commonly encountered while quickly working myself > through large PDFs with many pages (such as > [this](https://storage.ninetailed.ninja/index.php/s/FS58uge8yKCvjlB)). > > Steps to reproduce the issue: > > 1. Open Okular > 2. For easily reproducing the issue change *Settings* ? *Performance* ? > *Memory Usage* to *Greedy* (the issue can be reproduced without it, but it's > harder an alot more random) > 3. Open a PDF with many pages > 4. Immidiately go to presentation mode (*Ctrl*+*Shift*+*P*) > 5. Click on the screen or press the left-arrow keyboard button > > Expected behaviour: > > * The next slide is shown > > Actual behaviour: > > * The next slide *is* indeed shown for a split second before the one is sent > back to the first slide (this keeps being the case until all slides have been > preloaded, then it stops) > * Interestingly enough (and I frankly have no idea why) everything works > normally if one quickly double-clicks to skip forward by two slides, slides > can be viewed normally after that as long as you don't go back to the second > slide again…? > > > This patch fixes the problematic behaviour by removing code that looks its a > bogus fixi from something that shouldn't be a problem… maybe. > Unfortunately the one attached comment isn't helpful at all in describing > what this check was originally supposed to do – just like the commit message > for that change (obtained through `git blame`): > > commit d2d2fa3b2a4fa8f4b554b87ec13ee5a15b64f35d > Author: Mailson Menezes > Date: Sun Nov 11 23:09:12 2012 -0300 > > Refactoring PageView > > I therefor can only hope that this solution doesn't break anything. Guidance > of somebody with in-depth knowledge of the code-base would be apprechiated on > this. :-) > > > Diffs > - > > ui/pageview.cpp acacfb90 > > Diff: https://git.reviewboard.kde.org/r/130171/diff/ > > > Testing > --- > > Loaded a few different PDFs that had previously exibited the described > behaviour and verfied that they are fixed now. Also scrolled through them in > non-presentation mode, to verify that nothing else appears to be broken now. > > > Thanks, > > Alexander Schlarb > >
Review Request 130172: Add support for the forward and back mouse buttons
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130172/ --- Review request for Okular. Repository: okular Description --- This one should be a lot less controversial ;-) More information on this subject: https://doc.qt.io/qt-5/qt.html#MouseButton-enum Diffs - ui/presentationwidget.cpp 8035d38f Diff: https://git.reviewboard.kde.org/r/130172/diff/ Testing --- Yup, it works Thanks, Alexander Schlarb
Re: Review Request 130171: Fix page-jumping-back-issue commonly encountered when in presentation mode
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130171/#review103404 --- Sorry but no, you can't fix presentation mode by removing code out of pageview that you claim you don't know what it does. Debug more and come up with a better patch. - Albert Astals Cid On jul. 3, 2017, 8:54 p.m., Alexander Schlarb wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/130171/ > --- > > (Updated jul. 3, 2017, 8:54 p.m.) > > > Review request for Okular. > > > Repository: okular > > > Description > --- > > This fixes an issue I've commonly encountered while quickly working myself > through large PDFs with many pages (such as > [this](https://storage.ninetailed.ninja/index.php/s/FS58uge8yKCvjlB)). > > Steps to reproduce the issue: > > 1. Open Okular > 2. For easily reproducing the issue change *Settings* ? *Performance* ? > *Memory Usage* to *Greedy* (the issue can be reproduced without it, but it's > harder an alot more random) > 3. Open a PDF with many pages > 4. Immidiately go to presentation mode (*Ctrl*+*Shift*+*P*) > 5. Click on the screen or press the left-arrow keyboard button > > Expected behaviour: > > * The next slide is shown > > Actual behaviour: > > * The next slide *is* indeed shown for a split second before the one is sent > back to the first slide (this keeps being the case until all slides have been > preloaded, then it stops) > * Interestingly enough (and I frankly have no idea why) everything works > normally if one quickly double-clicks to skip forward by two slides, slides > can be viewed normally after that as long as you don't go back to the second > slide again…? > > > This patch fixes the problematic behaviour by removing code that looks its a > bogus fixi from something that shouldn't be a problem… maybe. > Unfortunately the one attached comment isn't helpful at all in describing > what this check was originally supposed to do – just like the commit message > for that change (obtained through `git blame`): > > commit d2d2fa3b2a4fa8f4b554b87ec13ee5a15b64f35d > Author: Mailson Menezes > Date: Sun Nov 11 23:09:12 2012 -0300 > > Refactoring PageView > > I therefor can only hope that this solution doesn't break anything. Guidance > of somebody with in-depth knowledge of the code-base would be apprechiated on > this. :-) > > > Diffs > - > > ui/pageview.cpp acacfb90 > > Diff: https://git.reviewboard.kde.org/r/130171/diff/ > > > Testing > --- > > Loaded a few different PDFs that had previously exibited the described > behaviour and verfied that they are fixed now. Also scrolled through them in > non-presentation mode, to verify that nothing else appears to be broken now. > > > Thanks, > > Alexander Schlarb > >
Review Request 130171: Fix page-jumping-back-issue commonly encountered when in presentation mode
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/130171/ --- Review request for Okular. Repository: okular Description --- This fixes an issue I've commonly encountered while quickly working myself through large PDFs with many pages (such as [this](https://storage.ninetailed.ninja/index.php/s/FS58uge8yKCvjlB)). Steps to reproduce the issue: 1. Open Okular 2. For easily reproducing the issue change *Settings* ? *Performance* ? *Memory Usage* to *Greedy* (the issue can be reproduced without it, but it's harder an alot more random) 3. Open a PDF with many pages 4. Immidiately go to presentation mode (*Ctrl*+*Shift*+*P*) 5. Click on the screen or press the left-arrow keyboard button Expected behaviour: * The next slide is shown Actual behaviour: * The next slide *is* indeed shown for a split second before the one is sent back to the first slide (this keeps being the case until all slides have been preloaded, then it stops) * Interestingly enough (and I frankly have no idea why) everything works normally if one quickly double-clicks to skip forward by two slides, slides can be viewed normally after that as long as you don't go back to the second slide again…? This patch fixes the problematic behaviour by removing code that looks its a bogus fixi from something that shouldn't be a problem… maybe. Unfortunately the one attached comment isn't helpful at all in describing what this check was originally supposed to do – just like the commit message for that change (obtained through `git blame`): commit d2d2fa3b2a4fa8f4b554b87ec13ee5a15b64f35d Author: Mailson Menezes Date: Sun Nov 11 23:09:12 2012 -0300 Refactoring PageView I therefor can only hope that this solution doesn't break anything. Guidance of somebody with in-depth knowledge of the code-base would be apprechiated on this. :-) Diffs - ui/pageview.cpp acacfb90 Diff: https://git.reviewboard.kde.org/r/130171/diff/ Testing --- Loaded a few different PDFs that had previously exibited the described behaviour and verfied that they are fixed now. Also scrolled through them in non-presentation mode, to verify that nothing else appears to be broken now. Thanks, Alexander Schlarb
D6268: HiDPI Support for Okular
davidedmundson added a comment. qApp->DPR can change within the lifespan of the app; for example plugging in a new monitor can change things. This does mean it's very important to keep the metadata of the pixmap's dpr with the pixmap. It ends up simpler and safer overall. Doesn't mean we need to break ABI though. --- @hetzenecker There are two breaks RequestPixmap::RequestPixmap and Page::_o_nearest --- For requestPixmap we're sure to not process any screen events in the meantime, so you can use qApp->devicePixelRatio() in the two places in document and in the line in generator. For use of Page::_o_nearest you can't. However, there is an easy way to keep ABI for this sort of thing. If you have someMethod(int a) and want to change it to someMethod(int a, qreal dpr) keep both, but make the first method simply call the second with a default parameter. - INLINE COMMENTS > generator.cpp:106 > +QPixmap *p = new QPixmap( QPixmap::fromImage( img ) ); > +p->setDevicePixelRatio( p->devicePixelRatioF() ); > +request->page()->setPixmap( request->observer(), p, > request->normalizedRect() ); sure about this one? REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, aacid
D6268: HiDPI Support for Okular
hetzenecker added a comment. Thanke for your feedback. In https://phabricator.kde.org/D6268#121135, @aacid wrote: > I would very much prefer if you didn't break the ABI of the files in core/ > > Also not sure why you need to pass the devicepixel ratio up and down, isn't it the same basically in all the app? Yes and no. While it would be possible to use `qApp->devicePixelRatio()` (as I've done in `mobile/components`), this would return the highest pixel ratio in the system, in case there are more physical screens with different Scale Factors connected. So, as alternative `QScreen::devicePixelRatio` could be used to return the DPR of the screen the application is shown. I'm not sure yet if this is possible without changing the ABI, but I'll try to. REPOSITORY R223 Okular REVISION DETAIL https://phabricator.kde.org/D6268 To: hetzenecker, davidedmundson, aacid Cc: sander, anthonyfieroni, #okular, aacid
