D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama added a comment.


  I will investigate more to find out why m_rects and m_highlights need 
different transformation matrices.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular, aacid
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Albert Astals Cid]

This revision was automatically updated to reflect the committed changes.
Closed by commit R223:6a2ed4f3144b: [Okular] Bug 387282: Highlighting of search 
results lost when rotating page (authored by ahmadosama, committed by aacid).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11901?vs=31856=32340#toc

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11901?vs=31856=32340

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

AFFECTED FILES
  core/page.cpp

To: ahmadosama, #okular, aacid
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Albert Astals Cid]

aacid accepted this revision.
aacid added a comment.
This revision is now accepted and ready to land.


  Looks like it works, i'll commit in a minute (with a small improvement from 
my side).
  
  I'd still like to know why m_rects and m_highlights need different 
transformation matrices.

REPOSITORY
  R223 Okular

BRANCH
  HighlightTextBug (branched from master)

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

To: ahmadosama, #okular, aacid
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama updated this revision to Diff 31856.
ahmadosama added a comment.


  I used Okular::buildRotationMatrix to rotate the highlights, the rotation 
angle is specified in the rotateAt function

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11901?vs=31291=31856

BRANCH
  HighlightTextBug (branched from master)

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

AFFECTED FILES
  core/page.cpp

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama added a comment.


  As far as I know rotating m_page->m_rects should rotate annotations on the 
page, the annotations seem to work well when rotating them.
  
  When rotating the highlights I tried to replace:
  (*hlIt)->transform( RotationJob::rotationMatrix( oldRotation, m_rotation ) );
  with
  (*hlIt)->transform( matrix );
  
  but highlights were rotated correctly only from the original orientation.
  
  I am thinking of creating another function to get the rotation matrix for the 
highlights, or modify the Okular::buildRotationMatrix
  as follows Okular::buildRotationMatrix(Rotation oldRotation = Rotation0, 
Rotation newRotation)
  
  I am still going through the code to find a better way to solve this bug.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama added a comment.


  I have not tested it, I do not know what on the page should be rotated when 
rotating m_page->m_rects, I still trying to find out.
  What I did is that I replaced:
   (*hlIt)->transform( RotationJob::rotationMatrix( oldRotation, m_rotation ) );
  with:
  (*hlIt)->transform( matrix );
  but the highlights were rotated correctly only from the original position.
  
  But, I think that m_page->m_rects may not be rotated correctly because 
Okular::buildRotationMatrix(Rotation rotation), rotates only based on the new 
rotation and does not consider what the previous rotation was.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Albert Astals Cid]

aacid added a comment.


  In D11901#241751 , @ahmadosama 
wrote:
  
  > I tried rotating using ( const QTransform matrix = rotationMatrix() ) but 
it only works when rotating a page from its original orientation, this is why I 
modified the RotationJob::rotationMatrix in a way similar to 
Okular::buildRotationMatrix to allow rotating from different orientations.
  
  
  So you mean that the rects in  m_page->m_rects are not being rotated 
correctly either?

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama added a comment.


  I tried rotating using ( const QTransform matrix = rotationMatrix() ) but it 
only works when rotating a page from its original orientation, this is why I 
modified the RotationJob::rotationMatrix in a way similar to 
Okular::buildRotationMatrix to allow rotating from different orientations.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Albert Astals Cid]

aacid added a comment.


  In D11901#239734 , @ahmadosama 
wrote:
  
  > Oh yes this may break it, I am thinking of adding another Boolean parameter 
to the RotationJob::rotationMatrix function with a default value of false to 
either translate or not
  >  The function can be:
  >  RotationJob::rotationMatrix( Rotation from, Rotation to, bool translate = 
false )
  
  
  I don't think that's a good idea, also the translations are a bit "random", 
why are they only 0 or 1?, i guess because you know the HighlightAreaRect are 
from 0 to 1, but that's knowledge that it's a bit "extra" to the rotationMatrix 
function.
  
  Can you check if we simply should not be using RotationJob::rotationMatrix? 
the other objects in the loop above are transformed using a different matrix 
and those ones seem to work? Maybe you can use that one?

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama updated this revision to Diff 31291.
ahmadosama added a comment.


  I added the boolean parameter to either translate or not.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11901?vs=31202=31291

BRANCH
  HighlightTextBug (branched from master)

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

AFFECTED FILES
  core/page.cpp
  core/rotationjob.cpp
  core/rotationjob_p.h

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama added a comment.


  Oh yes this may break it, I am thinking of adding another Boolean parameter 
to the RotationJob::rotationMatrix function with a default value of false to 
either translate or not
  The function can be:
  RotationJob::rotationMatrix( Rotation from, Rotation to, bool translate = 
false )

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Albert Astals Cid]

aacid added a comment.


  Isn't that translation breaking
  
  RotationJobInternal::run
  
  ?
  
  Why would you translate the image there?

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: aacid, ngraham, #okular, michaelweghorn

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama added a reviewer: Okular.

REPOSITORY
  R223 Okular

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

To: ahmadosama, #okular
Cc: #okular, michaelweghorn, ngraham, aacid

D11901: [Okular] Bug 387282: Highlighting of search results lost when rotating page

[Ahmad Osama]

ahmadosama created this revision.
Restricted Application added a subscriber: Okular.
Restricted Application added a project: Okular.
ahmadosama requested review of this revision.

REVISION SUMMARY
  The highlights were removed when rotating the page, also the 
RotaionJob::rotationMatrix function was not rotating the Highlihgts correctly.
  I removed the deletion and modified the rotaionMatrix function by making it 
shorter and adding a translation after the rotatin. I tried different rotations 
and they are working fine.
  BUG: 387282

REPOSITORY
  R223 Okular

BRANCH
  HighlightTextBug (branched from master)

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

AFFECTED FILES
  core/page.cpp
  core/rotationjob.cpp

To: ahmadosama
Cc: #okular, michaelweghorn, ngraham, aacid