D7671: Fix automatic reload of files saved with QSaveFile

2017-09-06 Thread Julian Wolff 
progwolff added a comment.


  In https://phabricator.kde.org/D7671#143269, @aacid wrote:
  
  > Have you read my email? There clearly says what happens and what the 
documentation says it should happen (at least to my understanding of reading 
it).
  
  
  Sure, I read your mail. But I still don't think that KDirWatch behaves wrong.
  On saving a file with QSaveFile or on a plain `mv somefile watchedfile` the 
watched file is never removed. As the docs say, the dirty signal of a directory 
is emitted when a file in this directory is removed or created. This is not the 
case here.
  KDirWatch yet does send a "created" signal on inotify's MOVE_TO flag and a 
"removed" signal on inotify's MOVE_FROM flag. This does not mean, the file 
actually has been removed or added at any time.
  
  So, this behaviour is a little strange and confusing, but from my perspective 
it's still coherent with the documentation.
  
  Even if KDirWatch would work as you expect it to, there are cases where 
Okular still does not react to those signals:
  Consider the command `rm watchedfile && touch watchedfile`.
  KDirWatch will send the signals: file removed, directory dirty, file added, 
directory dirty.
  Okular receives the first dirty signal asynchronously. Okular checks if the 
file exists via `QFile::exists`. It is likely that the file has already been 
added (`touch`) by that time, so Okular will miss to reload the file.
  
  ---
  
  In https://phabricator.kde.org/D7671#143360, @rkflx wrote:
  
  > Concerning 
https://phabricator.kde.org/R223:f93ccd7923491c6b1412ba5cb1fe0711e44496d8, I 
find the part re-adding the watch for removed files is also not needed with 
https://phabricator.kde.org/D7671.
  
  
  I agree. I think KDirWatch does this internally. Something that could be 
mentioned in the docs too.
  
  In https://phabricator.kde.org/D7671#143360, @rkflx wrote:
  
  > Here's my suggestion going forward:
  >
  > - Independently from Okular's case, evaluate accuracy of KDirWatch 
autotests and docs regarding "move/rename" (currently not mentioned at all, 
thus to be considered undefined behaviour…) as well as directory operations in 
general, fix potential code bugs and doc confusions
  > - Agree to accept https://phabricator.kde.org/D7671 in any case (reasons: 
code is simpler and less error-prone, fixes the issue even for users on LTS 
distros with old KF5 libs)
  > - Try to revert 
https://phabricator.kde.org/R223:f93ccd7923491c6b1412ba5cb1fe0711e44496d8 
including later fixups
  >
  >   TL;DR: Do both: Fix KDirWatch and apply https://phabricator.kde.org/D7671.
  
  
  I'd be perfectly happy with this. It's probably a good idea to recheck the 
docs of KDirWatch and mention the move_to and move_from cases there.
  Thanks for your constructive participation!

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D7671

To: progwolff, aacid
Cc: sander, rkflx, #okular, aacid

[okular] [Bug 362856] [Frameworks] Wrong render resolution, possibly caused by Plasma 5 "Scale Display"

2017-09-06 Thread Fest 
https://bugs.kde.org/show_bug.cgi?id=362856

Fest  changed:

   What|Removed |Added

 CC||fest...@gmail.com

-- 
You are receiving this mail because:
You are the assignee for the bug.

D7688: Implement rasterized printing with QPrinter with hidden annotations

2017-09-06 Thread Oliver Sander 
sander updated this revision to Diff 19240.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7688?vs=19165&id=19240

REVISION DETAIL
  https://phabricator.kde.org/D7688

AFFECTED FILES
  generators/poppler/CMakeLists.txt
  generators/poppler/config-okular-poppler.h.cmake
  generators/poppler/generator_pdf.cpp

To: sander, #okular
Cc: aacid

D7688: Implement rasterized printing with QPrinter with hidden annotations

2017-09-06 Thread Oliver Sander 
sander added a comment.


  Indeed.  I was somehow expecting this to happen automatically. :-)

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D7688

To: sander, #okular
Cc: aacid

D7688: Implement rasterized printing with QPrinter with hidden annotations

2017-09-06 Thread Albert Astals Cid 
aacid accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D7688

To: sander, #okular, aacid
Cc: aacid

D7671: Fix automatic reload of files saved with QSaveFile

2017-09-06 Thread Albert Astals Cid 
aacid added a comment.


  In https://phabricator.kde.org/D7671#143360, @rkflx wrote:
  
  > In https://phabricator.kde.org/D7671#143117, @aacid wrote:
  >
  > > It was added to make this work, and it did work at some point, i don't 
add code for nothing ;)
  >
  >
  > Of course, and your code still works today: it fixes the rm/touch case 
(with https://phabricator.kde.org/D7671 not applied yet). I guess for Kate it 
broke in 2012 (your patch is from 2008) with the introduction of KSaveFile in 
https://phabricator.kde.org/R40:a188aa837b7e609a1d555414603fb8d2ed7d70fa. My 
question was more about why you went for the directory approach instead of just 
listening for `KDirWatch::created`.
  
  
  I don't know, that was long time ago.
  
  > Concerning 
https://phabricator.kde.org/R223:f93ccd7923491c6b1412ba5cb1fe0711e44496d8, I 
find the part re-adding the watch for removed files is also not needed with 
https://phabricator.kde.org/D7671. Will do some more testing over the weekend 
and maybe prepare a patch. (You can tell I'm not a fan of encoding state in 
bools with updates spread over the codebase. As far as I can see, in 
conjunction with the directory watch this led to several bugs, e.g. 384185, 
234139, 321880 and maybe more.)
  
  Because there's a much better way of encoding state that using variables that 
encode state, right?
  
  > Here's my suggestion going forward:
  > 
  > - Independently from Okular's case, evaluate accuracy of KDirWatch 
autotests and docs regarding "move/rename" (currently not mentioned at all, 
thus to be considered undefined behaviour…) as well as directory operations in 
general, fix potential code bugs and doc confusions
  > - Agree to accept https://phabricator.kde.org/D7671 in any case (reasons: 
code is simpler and less error-prone, fixes the issue even for users on LTS 
distros with old KF5 libs)
  > - Try to revert 
https://phabricator.kde.org/R223:f93ccd7923491c6b1412ba5cb1fe0711e44496d8 
including later fixups
  > 
  >   TL;DR: Do both: Fix KDirWatch and apply https://phabricator.kde.org/D7671.
  
  I'm hesitant of applying something that just workarounds a library not 
working.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D7671

To: progwolff, aacid
Cc: sander, rkflx, #okular, aacid

D7671: Fix automatic reload of files saved with QSaveFile

2017-09-06 Thread Albert Astals Cid 
aacid accepted this revision.
aacid added a comment.
This revision is now accepted and ready to land.


  In https://phabricator.kde.org/D7671#143401, @progwolff wrote:
  
  > In https://phabricator.kde.org/D7671#143269, @aacid wrote:
  >
  > > Have you read my email? There clearly says what happens and what the 
documentation says it should happen (at least to my understanding of reading 
it).
  >
  >
  > Sure, I read your mail. But I still don't think that KDirWatch behaves 
wrong.
  
  
  Why?
  
  > On saving a file with QSaveFile or on a plain `mv somefile watchedfile` the 
watched file is never removed.
  
  According to KDirwatch, it is
  
  > As the docs say, the dirty signal of a directory is emitted when a file in 
this directory is removed or created. This is not the case here.
  
  According to KDirwatch, it is
  
  > KDirWatch yet does send a "created" signal on inotify's MOVE_TO flag and a 
"removed" signal on inotify's MOVE_FROM flag. This does not mean, the file 
actually has been removed or added at any time.
  
  The file may "in reality never be creater or removed", but KDirWatch is 
sending the removed and created signals, so if the documentation says "dirty is 
triggered when the file is removed and created", well it should send the 
signal, otherwise it's pretty confusing, and i don't know why we're discussing 
this here instead of on the mailing list where i actually asked for 
clarification.
  
  > So, this behaviour is a little strange and confusing, but from my 
perspective it's still coherent with the documentation.
  
  I disagree.
  
  > Even if KDirWatch would work as you expect it to, there are cases where 
Okular still does not react to those signals:
  >  Consider the command `rm watchedfile && touch watchedfile`.
  >  KDirWatch will send the signals: file removed, directory dirty, file 
added, directory dirty.
  >  Okular receives the first dirty signal asynchronously. Okular checks if 
the file exists via `QFile::exists`. It is likely that the file has already 
been added (`touch`) by that time, so Okular will miss to reload the file.
  >  Actually, this case seems to work with your code, but I hope you 
understand what I want to say. It just doesn't cover every potential case. Same 
problems apply for `mv`.
  
  This is actually the only good explanation of why the code is not robust 
enough, you're right there's races involved in the logic. So yeah let's commit 
this fix then.

REPOSITORY
  R223 Okular

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D7671

To: progwolff, aacid
Cc: sander, rkflx, #okular, aacid

Re: Review Request 130057: Bug 288042 - Option to reset forms

2017-09-06 Thread Albert Astals Cid 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130057/#review103692
---



When you open https://okular.kde.org/stuff/forms-scribus.pdf and press clear 
forms it moves you to page 4 if you're on page 1, that is weird.

Also shouldn't the option be in edit, it is for sure not a "view" option, no?

- Albert Astals Cid


On jul. 5, 2017, 4:53 p.m., Gilbert Assaf wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130057/
> ---
> 
> (Updated jul. 5, 2017, 4:53 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Bugs: 288042
> http://bugs.kde.org/show_bug.cgi?id=288042
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Clear Contents of FormLineEdit, TextAreaEdit and FileEdit + unchecks 
> CheckBoxEdit and ListEdit form elements. 
> 
> I don't touch RadioBoxes and ComboBoxes, because I am not sure, to which 
> state I should reset/clear them. Especially for ComboBox, I am not sure if 
> clearing them could be harmful.
> 
> 
> Diffs
> -
> 
>   part.rc 34a1160ef5d7b8f65e14a457f1b39bbb06fe3074 
>   ui/formwidgets.h b1bc11a4f7048935614ad384cc49dd1cd2b1336c 
>   ui/formwidgets.cpp 6ab49b0ed9fa3ba71486cf45e89e46c7b0f61d3f 
>   ui/pageview.h 43b03b0f677781cb92402e8291335424da210b65 
>   ui/pageview.cpp acacfb9047e678b99bb8a1cdcefbfeac0ac9c3d1 
>   ui/pageviewutils.h ca8bd78a4d78cb2e779b5b682c66fa60bcb3af80 
>   ui/pageviewutils.cpp a57712ca235b2ca629f5739ad73488d513416d0c 
> 
> Diff: https://git.reviewboard.kde.org/r/130057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Assaf
> 
>

Re: Review Request 130226: CHM Generator Lib Update

2017-09-06 Thread Albert Astals Cid 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130226/#review103693
---


Ship it!




I don't have many files to test not many knowledge of this but in general 
sounds good.

Will you commit to master?

- Albert Astals Cid


On ago. 26, 2017, 5:49 p.m., Gilbert Assaf wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130226/
> ---
> 
> (Updated ago. 26, 2017, 5:49 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> This patch updates our copy of the chm lib from kchmviewer. The lib files 
> itself are an unmodified copy from kchmviewer 7.7, only our generator needed 
> some changes. In contrast to kchmviewer we still use khtml and therefor still 
> need msits.
> 
> 
> Diffs
> -
> 
>   cmake/modules/FindLibZip.cmake PRE-CREATION 
>   generators/CMakeLists.txt 5eedf4ebb61237f92d5bbf64c216123140d27fd3 
>   generators/chm/CMakeLists.txt 83abe4e24e03b24622117156badb76a1b9d735da 
>   generators/chm/generator_chm.h 1485bc8aae60d662dfc0c01afa2f664dbba6382f 
>   generators/chm/generator_chm.cpp b6a770ece0d46cb7874bfdf388bae8074d240149 
>   generators/chm/lib/bitfiddle.h eb15b0fa9b0d13b27170be76828631d8328b3109 
>   generators/chm/lib/ebook.h PRE-CREATION 
>   generators/chm/lib/ebook.cpp PRE-CREATION 
>   generators/chm/lib/ebook_chm.h PRE-CREATION 
>   generators/chm/lib/ebook_chm.cpp PRE-CREATION 
>   generators/chm/lib/ebook_chm_encoding.h PRE-CREATION 
>   generators/chm/lib/ebook_chm_encoding.cpp PRE-CREATION 
>   generators/chm/lib/ebook_epub.h PRE-CREATION 
>   generators/chm/lib/ebook_epub.cpp PRE-CREATION 
>   generators/chm/lib/ebook_search.h PRE-CREATION 
>   generators/chm/lib/ebook_search.cpp PRE-CREATION 
>   generators/chm/lib/ebook_url.h PRE-CREATION 
>   generators/chm/lib/helper_entitydecoder.h PRE-CREATION 
>   generators/chm/lib/helper_entitydecoder.cpp PRE-CREATION 
>   generators/chm/lib/helper_search_index.h PRE-CREATION 
>   generators/chm/lib/helper_search_index.cpp PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontainer.h PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontainer.cpp PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontent.h PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubcontent.cpp PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubtoc.h PRE-CREATION 
>   generators/chm/lib/helperxmlhandler_epubtoc.cpp PRE-CREATION 
>   generators/chm/lib/lchmurlhandler.h 
> 35133c41d764de551a350a240d8ee43d07f84716 
>   generators/chm/lib/lchmurlhandler.cpp 
> 9d98d87e147539ef30817b3c66a040aa750575ad 
>   generators/chm/lib/libchmfile.h cb739ac7914d4856a5f0e8e6793d78e68b0c9628 
>   generators/chm/lib/libchmfile.cpp 60d03bc267eff759495af44b333448a55071b023 
>   generators/chm/lib/libchmfile_search.cpp 
> 76532b18282913ff97c8509d8e384ec0b1f48dfb 
>   generators/chm/lib/libchmfileimpl.h 
> f8d7cc11269a2688fd8b58a30d718ff911b051b3 
>   generators/chm/lib/libchmfileimpl.cpp 
> d10602028e958e7feded362b2ab58e32ca1d1ff0 
>   generators/chm/lib/libchmtextencoding.h 
> 5228b04c10718d407fc49df2e69d220d4efff67c 
>   generators/chm/lib/libchmtextencoding.cpp 
> 0ed3f0710360c7bab113a35c7896e6bbd6664d20 
>   generators/chm/lib/libchmtocimage.h 
> c0d98b3ba27596a731fd0ab24386578f6c58fdf8 
>   generators/chm/lib/libchmtocimage.cpp 
> 2952e8604d8c01360eace2826bbf5dc428155ff1 
> 
> Diff: https://git.reviewboard.kde.org/r/130226/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Assaf
> 
>

Re: Review Request 128858: [frameworks] Hide cursor when tablet pen leaves proximity of the screen

2017-09-06 Thread Albert Astals Cid 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128858/#review103694
---




ui/presentationwidget.cpp (line 1305)


Why this change?


- Albert Astals Cid


On oct. 5, 2016, 4:20 p.m., Oliver Sander wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128858/
> ---
> 
> (Updated oct. 5, 2016, 4:20 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> I am trying to fix the following papercut:  I regularly write on pdf files in 
> presentation mode, using the pen that comes with my Lenovo ThinkPad Yoga.  
> When I approach the screen with the pen, the cursor appears, and it follows 
> the pen tip during writing.  When I lift off the pen, the cursor stays on, 
> and auto-hides only a few seconds later (because of 
> Okular::Settings::EnumSlidesCursor::HiddenDelay).  As a consequence, the 
> cursor frequently hides the last bits of what I have just written.  This is a 
> nuisance, because I do this in front of an audience, and a lot of it is math 
> (where every detail matters).
> 
> Ideally, the cursor would auto-hide when I lift the pen off the screen.  
> Luckily, Qt has an event for this: QEvent::TabletLeaveProximity.  Unluckily, 
> the documentation says (http://doc.qt.io/qt-5/qtabletevent.html):
> 
> "TabletEnterProximity and TabletLeaveProximity events [...] are only sent to 
> QApplication"
> 
> Therefore, this patch introduces a new class TabletApplication, which 
> inherits from QApplication, and is used in main.cpp instead of QApplication.  
> The proximity events are really caught, and each time a short note is printed 
> on the console.
> 
> Unfortunately, at this point I am stuck and need some help.  Apparently, I 
> cannot control the cursor from a QApplication.  How do I get the information 
> that a TabletProximity has been caught to the presentation widget? Maybe the 
> answer is trivial, but I have very little Qt programming experience.  Thanks 
> for your help!
> 
> 
> Diffs
> -
> 
>   ui/presentationwidget.h 69574d2 
>   ui/presentationwidget.cpp c16d616 
> 
> Diff: https://git.reviewboard.kde.org/r/128858/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Oliver Sander
> 
>

D7714: Show dialog to ask when closing when more than tab open

2017-09-06 Thread Albert Astals Cid 
aacid created this revision.
Restricted Application added a subscriber: Okular.
Restricted Application added a project: Okular.

REVISION SUMMARY
  The checkbox is checked and says "Warn me on closing more than one tab",
  for that reason we can't use the default KMessageBox::questionYesNo since
  there the checkbox is always not checked and it's when checked that you 
enable it
  
  Inspired by code from torham zed torham...@yahoo.com at review request 126406

REPOSITORY
  R223 Okular

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D7714

AFFECTED FILES
  shell/shell.cpp

To: aacid
Cc: #okular, aacid

Re: Review Request 126406: Okular should warn if closing with multiple tabs opened

2017-09-06 Thread Albert Astals Cid 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126406/#review103695
---



I'm going to close this one in favour of https://phabricator.kde.org/D7714 
please keep the discussion there.

- Albert Astals Cid


On ago. 6, 2016, 1:14 a.m., torham zed wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126406/
> ---
> 
> (Updated ago. 6, 2016, 1:14 a.m.)
> 
> 
> Review request for Okular and Albert Astals Cid.
> 
> 
> Bugs: 334019
> http://bugs.kde.org/show_bug.cgi?id=334019
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> Multi tab popup warning
> 
> 
> Diffs
> -
> 
>   home/user/projects/OkularOriginal/shell/shell.cpp d80def4 
> 
> Diff: https://git.reviewboard.kde.org/r/126406/diff/
> 
> 
> Testing
> ---
> 
> Opened several pdf files, closed entire program with multiple files open to 
> see if popup properly functions along with the 'yes' and 'no' buttons.
> 
> 
> Thanks,
> 
> torham zed
> 
>

Re: Review Request 128858: [frameworks] Hide cursor when tablet pen leaves proximity of the screen

2017-09-06 Thread Oliver Sander 


> On Sept. 6, 2017, 8:24 p.m., Albert Astals Cid wrote:
> > ui/presentationwidget.cpp, line 1305
> > 
> >
> > Why this change?

Previously, the code there was

  slotChangeDrawingToolEngine( QDomElement() );
  slotChangeDrawingToolEngine( m_currentDrawingToolElement );

And here is the content of slotChangeDrawingToolEngine:

if ( element.isNull() )
{
delete m_drawingEngine;
m_drawingEngine = 0;
m_drawingRect = QRect();
setCursor( Qt::ArrowCursor );
}
else
{
m_drawingEngine = new SmoothPathEngine( element );
setCursor( QCursor( QPixmap( QStringLiteral("pencil") ), 
Qt::ArrowCursor ) );
m_currentDrawingToolElement = element;
}

You can see that it sets the cursor shape, but we don't want that: When drawing 
with the stylus we want the cursor to remain a CrossCursor, not change back to 
ArrowCursor.  But the cursor change is really only a by-product of the calls to 
this method.  Therefore I simply inlined the calls to 
slotChangeDrawingToolEngine, and omitted the cursor change.


- Oliver


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128858/#review103694
---


On Oct. 5, 2016, 4:20 p.m., Oliver Sander wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128858/
> ---
> 
> (Updated Oct. 5, 2016, 4:20 p.m.)
> 
> 
> Review request for Okular.
> 
> 
> Repository: okular
> 
> 
> Description
> ---
> 
> I am trying to fix the following papercut:  I regularly write on pdf files in 
> presentation mode, using the pen that comes with my Lenovo ThinkPad Yoga.  
> When I approach the screen with the pen, the cursor appears, and it follows 
> the pen tip during writing.  When I lift off the pen, the cursor stays on, 
> and auto-hides only a few seconds later (because of 
> Okular::Settings::EnumSlidesCursor::HiddenDelay).  As a consequence, the 
> cursor frequently hides the last bits of what I have just written.  This is a 
> nuisance, because I do this in front of an audience, and a lot of it is math 
> (where every detail matters).
> 
> Ideally, the cursor would auto-hide when I lift the pen off the screen.  
> Luckily, Qt has an event for this: QEvent::TabletLeaveProximity.  Unluckily, 
> the documentation says (http://doc.qt.io/qt-5/qtabletevent.html):
> 
> "TabletEnterProximity and TabletLeaveProximity events [...] are only sent to 
> QApplication"
> 
> Therefore, this patch introduces a new class TabletApplication, which 
> inherits from QApplication, and is used in main.cpp instead of QApplication.  
> The proximity events are really caught, and each time a short note is printed 
> on the console.
> 
> Unfortunately, at this point I am stuck and need some help.  Apparently, I 
> cannot control the cursor from a QApplication.  How do I get the information 
> that a TabletProximity has been caught to the presentation widget? Maybe the 
> answer is trivial, but I have very little Qt programming experience.  Thanks 
> for your help!
> 
> 
> Diffs
> -
> 
>   ui/presentationwidget.h 69574d2 
>   ui/presentationwidget.cpp c16d616 
> 
> Diff: https://git.reviewboard.kde.org/r/128858/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Oliver Sander
> 
>