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

Authored by shubham on Tue, Feb 5, 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 or press combination of ALT and P to select the entire page. Then the selected text can be copied using context menu event entry named "Copy Text"

Diff Detail

Repository
R223 Okular
Branch
arcpatch-D18744
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8437
Build 8455: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a project: Okular. · View Herald TranscriptTue, Feb 5, 6:43 AM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
shubham requested review of this revision.Tue, Feb 5, 6:43 AM
shubham added a subscriber: Okular.Tue, Feb 5, 6:43 AM
Restricted Application removed a subscriber: Okular. · View Herald TranscriptTue, Feb 5, 6:43 AM
shubham added a comment.EditedTue, Feb 5, 6:47 AM

Note: This is not a working version, see my inline comment. However, I can confirm that after fixing the seg fault error, it will become working since I have manually tested to select the entire page and then qDebug() the "text" gave same contents as that of the selected page.

shubham added inline comments.Tue, Feb 5, 6:49 AM
ui/pageview.cpp
3323

Lines below produce segmentation fault, can't figure out what's wrong here

shubham added inline comments.Tue, Feb 5, 7:17 AM
ui/pageview.cpp
3323

This part of code actually just visualy selects the page, so that a context menu event can occur.

shubham updated this revision to Diff 50958.EditedTue, Feb 5, 2:55 PM

Please ignore my above comments.

Fix segmentation fault error.
Now mouse double click event on empty portion of the document selects the entire page

shubham marked 2 inline comments as done.Tue, Feb 5, 2:56 PM
shubham retitled this revision from [WIP]Select entire page with double click to [WIP]Select entire page on mouse double click event.
shubham updated this revision to Diff 50959.Tue, Feb 5, 2:57 PM

Remove unncessary <QDebug> include

shubham retitled this revision from [WIP]Select entire page on mouse double click event to Select entire page on mouse double click event.Tue, Feb 5, 2:59 PM
shubham edited the test plan for this revision. (Show Details)
ngraham edited reviewers, added: VDG; removed: ngraham.Tue, Feb 5, 4:00 PM

Interesting feature. Can confirm that it works for me; tested with a bunch of PDFs and ePubs with selectable text. No effect with documents that don't have selectable text. No crashes.

Seems sane enough to me. However I wonder if maybe triple-click might be better though? Triple-click is used for "select everything" in word processors, so perhaps we could take advantage of some familiarity there?

In the future for user interface or user-facing changes, please add VDG instead of me directly (that way other UI and UX designers can see it too)

Seems sane enough to me. However I wonder if maybe triple-click might be better though? Triple-click is used for "select everything" in word processors, so perhaps we could take advantage of some familiarity there?

A single click to set the cursor, a double click to the select the current word under the cursor and a triple click to select a whole line is the standard behaviour in most applications I know.
Currently you can select everything by CTRL+A. So, maby for a whole single page, it could be CTRL+SHIFT+A? Or ALT+Triple Click?

loh.tar added a subscriber: loh.tar.Tue, Feb 5, 4:22 PM

I suggest:

  • Add a new action "Select Page" similar to "Select All"
  • Call first that new action/function, then the copy function, instead of coding explicit in mouse event
  • Shorten your comments, don't say what is obviously, explain things which may indistinct e.g."select the word which was clicked on" "select the entire page if click event is in empty portion"

I suggest:

  • Add a new action "Select Page" similar to "Select All"
  • Call first that new action/function, then the copy function, instead of coding explicit in mouse event
  • Shorten your comments, don't say what is obviously, explain things which may indistinct e.g."select the word which was clicked on" "select the entire page if click event is in empty portion"

+1

abetts added a subscriber: abetts.Tue, Feb 5, 5:11 PM

This idea is super interesting. Can you please make a gif or short video showing the new feature?

@ngraham @loh.tar I don't think it is that big of a feature to be shown in the view menu. It is something which is natural, same as when you select a word(which also don't have any action defined). i'm okay anyways.

shubham edited the test plan for this revision. (Show Details)Tue, Feb 5, 5:42 PM

Caution: I did not do my video editing magic : )

aacid added a comment.Tue, Feb 5, 6:45 PM

Honestly i think this is weird UI, no other single project out there does this (that i know)

Personally I would prefer if we didn't do it either.

However I wonder if maybe triple-click might be better though? Triple-click is used for "select everything" in word processors, so perhaps we could take advantage of some familiarity there?

I would prefer triple-click too, or a keyboard shortcut, as alexde suggested.

Selecting the whole page makes a big part of the screen flash, which is annoying when you accidentally do a double-click. This often happens with touchpads, when you want to drag something, but lose contact after the second tap. And to select a line with Text Selection Tool, you will need this double-click-drag.

aacid added a comment.Tue, Feb 5, 9:51 PM

However I wonder if maybe triple-click might be better though? Triple-click is used for "select everything" in word processors, so perhaps we could take advantage of some familiarity there?

I would prefer triple-click too, or a keyboard shortcut, as alexde suggested.

AFAICS triple-click is usually "select line" not "select all page/document"

Having triple click for "select line" would be actually a great improvement IMHO

Yeah, that would be nice.

Maybe select the entire page on quadruple-click? I agree that double-click is not ideal.

In D18744#406150, @aacidTriple-click is used for "select everything" in word processors, so perhaps we could take advantage of some familiarity there?

AFAICS triple-click is usually "select line" not "select all page/document"

Having triple click for "select line" would be actually a great improvement IMHO

I will submit a new patch for it.

I'm unsure how mouse click events of degree n can be handled. Maybe handle mouseDoubleClickEvent() nested inside another mouseDoubleClickEvent() ?

shubham edited the summary of this revision. (Show Details)Wed, Feb 6, 5:31 PM
shubham edited the summary of this revision. (Show Details)Wed, Feb 6, 5:49 PM
shubham updated this revision to Diff 51609.Wed, Feb 13, 5:05 PM
shubham edited the summary of this revision. (Show Details)

Add action and shortcut to select the current page
Remove triggering on mouseDoubleClick event

Restricted Application added a project: Documentation. · View Herald TranscriptWed, Feb 13, 5:05 PM
Restricted Application added a subscriber: kde-doc-english. · View Herald Transcript
shubham updated this revision to Diff 51610.Wed, Feb 13, 5:09 PM

Remove unneeded comment

shubham retitled this revision from Select entire page on mouse double click event to Add action in Edit menu to select the entire page .Wed, Feb 13, 5:11 PM
shubham retitled this revision from Add action in Edit menu to select the entire page to Add action in Edit menu to select the current page .
shubham edited the test plan for this revision. (Show Details)Wed, Feb 13, 5:23 PM
ngraham requested changes to this revision.Wed, Feb 13, 6:55 PM

Please test your changes. The new menu is not actually added to the Edit menu and the shortcut you chose conflicts with the print shortcut.

ui/pageview.cpp
740

This icon is already used for the Select All action, and we don't want to re-use the same icon for two actions in the same menu. Let's remove it.

741

The correct text here would be "Select all text on current page"

743

Erm, that's the shortcut used for printing. Did you test this?

This revision now requires changes to proceed.Wed, Feb 13, 6:55 PM
shubham added a comment.EditedWed, Feb 13, 7:07 PM

Please test your changes. The new menu is not actually added to the Edit menu and the shortcut you chose conflicts with the print shortcut.

I knew ctrl p is for print, so for time being I kept it so I can get suggestion for other shortcut sequence. Also, I can't tell why entry is not added into Edit menu even though I had added that in docbook.

This patch works with ctrl p selecting the text(I checked after changing default shortcut for Print), only thing being absence of the corresponding entry in Edit.

I knew ctrl p is for print, so for time being I kept it so I can get suggestion for other shortcut sequence.

It's not acceptable to deliberately publish a diff that does the wrong thing purely for the purpose of soliciting comments without adding [WIP] or [RFC] to the title. What if nobody noticed this and it landed as-is and broke printing?

Also, I can't tell why entry is not added into Edit menu even though I had added that in docbook.

The docbook is just for documentation. To put it in the menu structure, you need to add the action to part.rc and bump the version number at the top of the file.

shubham added inline comments.Thu, Feb 14, 2:59 AM
ui/pageview.cpp
743

What sequence should I use?

ngraham added inline comments.Thu, Feb 14, 3:12 AM
ui/pageview.cpp
743

It doesn't have a shortcut at all. If you can't think of anything that doesn't conflict, then just don't give it one.

shubham added inline comments.Thu, Feb 14, 3:13 AM
ui/pageview.cpp
743

So better I will remove that.

loh.tar added inline comments.Thu, Feb 14, 6:26 AM
ui/pageview.cpp
743

Here is e.g. Alt-P, and a couple of F-Keys not used.
But perhaps exist in other software some similar function with a usual key sequence that can be adopt

shubham updated this revision to Diff 51685.Thu, Feb 14, 2:47 PM
shubham marked 3 inline comments as done.
shubham retitled this revision from Add action in Edit menu to select the current page to Add action in Edit menu to select the current page.

Add action to part.rc
Bump version to 40

shubham retitled this revision from Add action in Edit menu to select the current page to [RFC]Add action in Edit menu to select the current page.Thu, Feb 14, 2:48 PM
shubham edited the test plan for this revision. (Show Details)Thu, Feb 14, 3:49 PM
ngraham resigned from this revision.Thu, Feb 14, 4:07 PM

OK, works for me now. I'm not thrilled about the Alt-P shortcut since it's the only one in the menu that uses only the Alt modifier. But I won't block on that. I'll hand this back to the Okular folks now.

shubham retitled this revision from [RFC]Add action in Edit menu to select the current page to Add action in Edit menu to select the current page.Thu, Feb 14, 4:43 PM

The test plan still says that Ctrl+P will select the page ("or press combination of CTRL and P to select the entire page"). As far as I understand the previous comments, this is Alt+P now, isn't it? (I didn't do any tests.) Can you update the description accordingly?

The test plan still says that Ctrl+P will select the page ("or press combination of CTRL and P to select the entire page"). As far as I understand the previous comments, this is Alt+P now, isn't it? (I didn't do any tests.) Can you update the description accordingly?

And as far as I can see, the same applies for the docbook.

shubham edited the test plan for this revision. (Show Details)Fri, Feb 15, 1:24 AM
shubham edited the test plan for this revision. (Show Details)Fri, Feb 15, 2:49 AM
shubham retitled this revision from Add action in Edit menu to select the current page to Add action in Edit menu to select the text on current page.Fri, Feb 15, 2:55 AM
shubham updated this revision to Diff 51829.Sat, Feb 16, 8:28 AM
shubham edited the test plan for this revision. (Show Details)

Correct shortcut in documentation
Fix coding style

OK, works for me now. I'm not thrilled about the Alt-P shortcut since it's the only one in the menu that uses only the Alt modifier. But I won't block on that. I'll hand this back to the Okular folks now.

Does that earn +1 or Accepted from VDG? ;)

aacid added a comment.EditedSat, Feb 16, 6:10 PM

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

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.EditedSun, Feb 17, 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.Sun, Feb 17, 4:47 PM

Remove shortcut and it's documentation