Add action in Edit menu to select the text on current page
ClosedPublic

Authored by shubham on Feb 5 2019, 6:43 AM.

Details

Summary

BUG: 358868

Test Plan

Click on "Select All Text on Current Page" entry in Edit menu to select the entire page. The selected text can then be copied via Edit menu item "Copy"

Diff Detail

Repository
R223 Okular
Branch
arcpatch-D18744
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10057
Build 10075: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

Remove shortcut and it's documentation

aacid added a comment.Feb 21 2019, 6:38 PM

Double-click in the margins can't be used for this since there would be too many accidental activations.

Here's my proposal

  • Double-click in the margins: select whole line
  • Triple-click in the margins or on a word: select whole paragraph
  • Quadruple-click in the margins: select whole page

what is a line or paragraph when the page has three columns of text? I don't think this makes sense

aacid added a comment.Feb 21 2019, 6:39 PM

@aacid ping?

Personally i'm happy with a menu option, "no one" will see it, but that's the point, it's an option that "no one" needs.

And for the people that need it, they'll search for it, google it, maybe even try to use the documentation and then they'll find it in the menu.

And then people that for some reason need to select pages very often and are smart will learn how to assign the shortcut they want.

ngraham requested changes to this revision.Feb 21 2019, 8:43 PM

All right, let's do this. In final testing, I just found some blockers:

  • If the current mouseMode is not TextSelect, then the menu item is enabled but does nothing. It should do its thing no matter what the mouseMode is, just like Select all does.
  • The menu item is still enabled when Okular is opened without a document, and clicking on it crashes Okular. It needs to be disabled when no document is open.
This revision now requires changes to proceed.Feb 21 2019, 8:43 PM
shubham edited the test plan for this revision. (Show Details)Feb 23 2019, 9:49 AM
shubham updated this revision to Diff 52502.Feb 25 2019, 1:40 AM

Enable action for all the modes
Try to fix crash

I can't test it since for me, somehow I don't see the action added to the menu.

Tested. Works as expected.

doc/index.docbook
1301

Trailing spaces

ui/pageview.cpp
737

Trailing spaces

743

Trailing spaces

5575

Trailing space

5579

Trailing spaces

5581

Trailing spaces

5587

Trailing space

5590

Trailing spaces

5596

Trailing spaces

ui/pageview.h
283

Trailing spaces

@yurchor what about the crash case, is it fixed now?

@yurchor what about the crash case, is it fixed now?

No. I think that the action should be created in part.cpp, setEnabled(false) by default, then setEnabled(true) in the Part::openFile to avoid the crash.

Regretfully, no function (like isOpened) for setEnabled to connect to.

@yurchor what about the crash case, is it fixed now?

No. I think that the action should be created in part.cpp, setEnabled(false) by default, then setEnabled(true) in the Part::openFile to avoid the crash.

Regretfully, no function (like isOpened) for setEnabled to connect to.

Another strategy can be to check for if doc is not opened and then setEnable(false)
Since setEnabled() is true by default, my if statement is basically dead code.

shubham updated this revision to Diff 52576.Feb 26 2019, 4:49 AM
  1. Try to fix crash case
  2. Improve comment
  1. Try to fix crash case
  2. Improve comment

Now it is impossible to reach the Edit menu item at all.

Can you please try the version of this patch (just a minor tweak) that works fine for me? Thanks in advance for testing.

shubham updated this revision to Diff 52729.Feb 27 2019, 2:48 PM

Now it's working for me without crashes. Thanks.

@aacid @ngraham Can you please confirm it now works to your expectations so that I can go ahead pushing this?

ngraham accepted this revision.Feb 28 2019, 2:12 PM

Yep, it works to my satisfaction now.

This revision is now accepted and ready to land.Feb 28 2019, 2:12 PM

I will wait for @aacid's review before landing it.

aacid added inline comments.Mar 5 2019, 11:47 PM
doc/index.docbook
1297

Does this need updating? this doesn't depend on the mode anymore, does it?

@shubham : Can you make this last minor fix in docs? Thanks in advance for your answer.

@ngraham : "Select All Text On Current Page" - Is this a correct letter casing/wording for the main menu item?

@shubham : Can you make this last minor fix in docs? Thanks in advance for your answer.

@ngraham : "Select All Text On Current Page" - Is this a correct letter casing/wording for the main menu item?

Yeah, this should probably be "Select All Text on Current Page"

shubham added a comment.EditedMar 10 2019, 6:27 PM

Surely I can do that, but currently I am busy with my exams. Give me a week's time.

shubham updated this revision to Diff 53794.Mar 13 2019, 2:09 PM
shubham marked an inline comment as done.

Make requested changes by Albert and Nate

shubham edited the test plan for this revision. (Show Details)Mar 13 2019, 2:09 PM
ngraham accepted this revision.Mar 13 2019, 3:04 PM
aacid requested changes to this revision.Mar 14 2019, 10:57 PM
aacid added inline comments.
ui/pageview.cpp
5598

Why this return?

This revision now requires changes to proceed.Mar 14 2019, 10:57 PM
shubham updated this revision to Diff 54493.Mar 21 2019, 4:21 PM

Remove useless return

shubham marked an inline comment as done.Mar 21 2019, 4:21 PM
ngraham requested changes to this revision.Mar 24 2019, 5:48 PM

I just re-tested and now the menu item crashes when used, no matter which PDF I try. :(

Again, please test before submitting or updating a patch.

This revision now requires changes to proceed.Mar 24 2019, 5:48 PM
shubham updated this revision to Diff 54700.EditedMar 24 2019, 6:00 PM

Try to fix the crash. I could not test it because for me I could not see the entry added to the menu.

Try to fix the crash. I could not test it because for me I could not see the entry added to the menu.

You gotta fix that. :) Try deleting ~/.local/share/kxmlgui5/okular/partrc Does that help?

shubham added inline comments.Mar 24 2019, 6:06 PM
ui/pageview.cpp
5598

If removed, it causes crash.

Try to fix the crash. I could not test it because for me I could not see the entry added to the menu.

You gotta fix that. :) Try deleting ~/.local/share/kxmlgui5/okular/partrc Does that help?

Thanks, that is helpful.

ngraham accepted this revision.Mar 24 2019, 6:07 PM
aacid requested changes to this revision.Mar 24 2019, 10:23 PM
aacid added inline comments.
ui/pageview.cpp
5598

Sorry but that's not a valid answer, you'll have to explain why sometimes you have to delete area and why sometimes not

This revision now requires changes to proceed.Mar 24 2019, 10:23 PM

@aacid area is a RegularAreaRect object which is a QRect bounding the selected text. "area" is to be deleted in all the cases except the case in which it is not possible to delete it since "area" will not contain any text in it and hence it will be nullptr. (case: if text is empty string) and hence return is called instead of deleting the "area".

@aacid area is a RegularAreaRect object which is a QRect bounding the selected text. "area" is to be deleted in all the cases except the case in which it is not possible to delete it since "area" will not contain any text in it and hence it will be nullptr. (case: if text is empty string) and hence return is called instead of deleting the "area".

Wait, stupid question: You assume area to be nullptr if there is no text on the page?

You do area = TextSelectionForItem( item );, PageView::TextSelectionForItem() unconditionally returns a Okular::RegularAreaRect * which it got from TextPage::TextArea(). TextPage::TextArea() does return new RegularAreaRect(); if TextSelectionForItem() is called on an empty page, and RegularAreaRect * ret= new RegularAreaRect; [complicated] return ret; [or] ret->addShape([...]); return ret otherwise.

TextSelectionForItem() tests whether it gets nullptr from TextArea(). But how can TextArea() return nullptr at all? new RegularAreaRect() vs. new RegularAreaRect?

besides "delete nullptr" is perfectly fine and non crashy.

shubham updated this revision to Diff 54854.Mar 26 2019, 1:44 PM

Do not delete area

aacid added inline comments.Mar 31 2019, 10:02 PM
ui/pageview.cpp
5587

I don't think we should be copying to the clipboard.

selectAll() isn't doing it, so this function doing it would be strage usability wise.

Do not delete area

Interestingly, setPageTextSelection() deletes area (without making it nullptr). Shouldn’t this side effect be documented (in another patch)?

ui/pageview.cpp
5587

The other selection tools copy to clipboard. Maybe Select All should do so too, because the selection can not be used any other way than Menu -> Edit -> Copy. (Other way: e. g. right click -> Search With DuckDuckGo)

aacid added a comment.Apr 2 2019, 10:02 PM

Do not delete area

Interestingly, setPageTextSelection() deletes area (without making it nullptr). Shouldn’t this side effect be documented (in another patch)?

Sure if you want to create a patch that documents it, feel free (BTW it can't set the pointer to null for the caller, that's not how pointers work)

ui/pageview.cpp
5587

Not convinced, for example kate doesn't do that either.

open kate, type something, press ctrl+a to select it, it only gets selected, not copied to the clipboard

but if you select it with the mouse it then gets copied to the clipboard.

It'd be the same we're doing, so i think this consistency is good to maintain.

I agree that it shouldn't copy automatically.

shubham edited the test plan for this revision. (Show Details)Apr 8 2019, 4:06 PM
shubham updated this revision to Diff 55753.Apr 8 2019, 4:09 PM
shubham edited the test plan for this revision. (Show Details)

Do not copy to clipboard

shubham marked an inline comment as done.Apr 8 2019, 4:09 PM
shubham edited the test plan for this revision. (Show Details)

You've already got my approval. :)

aacid accepted this revision.Apr 21 2019, 11:10 PM
This revision is now accepted and ready to land.Apr 21 2019, 11:10 PM
This revision was automatically updated to reflect the committed changes.