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

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

Details

Reviewers
aacid
ngraham
Group Reviewers
VDG
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 10622
Build 10640: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

To be honest, I just don't feel great about this. The original bug (select whole page's text on double-click) was a bad idea, and we've tried to torture this patch into implementing the requested feature in a sort of awkward way, with a top-level menu item in the Edit menu that has no icon and probably no keyboard shortcut--all for a function that's likely to be used infrequently--if ever--for most users.

I think I would feel better if we instead did the following:

  1. Triple-click to select current line or current paragraph
  2. Quadruple-click to select whole page

What do other folks think?

Do not use Alt+Letter for a shortcut, it's bad.

Because Alt is used for Access Keys (Mnemonics), right?

To be honest, I just don't feel great about this. The original bug (select whole page's text on double-click) was a bad idea, and we've tried to torture this patch into implementing the requested feature in a sort of awkward way, with a top-level menu item in the Edit menu that has no icon and probably no keyboard shortcut--all for a function that's likely to be used infrequently--if ever--for most users.

I think I would feel better if we instead did the following:

  1. Triple-click to select current line or current paragraph
  2. Quadruple-click to select whole page

    What do other folks think?

I thought, this should get quadruple-click and Ctrl+Shift+A.

Text Selection mode already pollutes the clipboard, so this action could also be called “Copy all text on current page”. And then, it would also make sense in the other modes, like Select All.

Considering the clipboard and how long Okular hangs to “select” a catalogue or datasheet with 1000+ pages, this action should get Ctrl+A. The other action would become “Select all Text on all Pages”, with Ctrl+Shift+A. This makes it less easy to hang Okular unintentionally.

And, how often would someone want to select the whole document? The result would include all headers, footers,... several times. This is mostly useful for .txt files that were converted to PDF. In a catalogue or instruction set, Select all Text on Current Page is more useful. Thus, the idea to select the page is not worse than to select the whole document.

Related:
Considering headers and footers, these probably should be exclude-able from both Select All and Find..., The easiest way is probably to ignore the trimmed-away margins.

shubham added a comment.EditedFeb 17 2019, 2:17 AM
  1. Quadruple-click to select whole page What do other folks think?

In my view quadruple click does not make sense because it is seldom the user can think it can result in some kind of action or behaviour.
Secondly, this feature then would remain hidden or unknown about, even though I originally implemented this in double click. I would prefer double click over quadruple since for me double click is expected to do something, but surely quadruple is not.(I don't know about others)
At the end you all are much more experienced amd understand UX way better than me, I will leave this to you.

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
shubham updated this revision to Diff 51907.Feb 17 2019, 4:47 PM

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.Sun, Mar 24, 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.Sun, Mar 24, 5:48 PM
shubham updated this revision to Diff 54700.EditedSun, Mar 24, 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.Sun, Mar 24, 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.Sun, Mar 24, 6:07 PM
aacid requested changes to this revision.Sun, Mar 24, 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.Sun, Mar 24, 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.Tue, Mar 26, 1:44 PM

Do not delete area

aacid added inline comments.Sun, Mar 31, 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.Tue, Apr 2, 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)Mon, Apr 8, 4:06 PM
shubham updated this revision to Diff 55753.Mon, Apr 8, 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.Mon, Apr 8, 4:09 PM
shubham edited the test plan for this revision. (Show Details)

You've already got my approval. :)