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:
- Triple-click to select current line or current paragraph
- Quadruple-click to select whole page
What do other folks think?
Because Alt is used for Access Keys (Mnemonics), right?
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.
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.
- 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
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.
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.
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.
@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?
Interestingly, setPageTextSelection() deletes area (without making it nullptr). Shouldn’t this side effect be documented (in another patch)?
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)
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)
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.