Allow scaling documentation view (BUG 285162)
ClosedPublic

Authored by igorkushnir on Oct 12 2016, 6:23 PM.

Details

Summary

Add ZoomController utility

Prevent future StandardDocumentationView ABI breakage

Implement Ctrl+mouse_scroll zoom in StandardDocumentationView

Eliminate a quite restrictive hard minimum documentation font size limit

Add ZoomActions utility

Show zoom actions in StandardDocumentationView context menu

Add default context menu actions getter to StandardDocumentationView

Test Plan
built, installed;
verified that Ctrl+mouse_scroll and context menu actions work;
verified that all the previously available context menu actions are preserved;
passed 3 related tests: test_cmakemanager, test_manpagemodel, test_qthelpplugin.

Diff Detail

Repository
R33 KDevPlatform
Branch
documentation-zoom-new-trimmed
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
igorkushnir updated this object.Oct 12 2016, 6:23 PM
igorkushnir edited the test plan for this revision. (Show Details)
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 12 2016, 6:23 PM

I tried to allow configuring documentation zoom actions shortcuts:

However I don't think that this patch is good enough, and I'm not sure if this configuration is necessary.

Patch advantages:

  1. Zoom shortcuts are configurable.
  2. One ZoomActions object is created at KDevelop start instead of creating a new object on each documentation provider switch or after returning to documentation home.

Patch disadvantages:

  1. It's unclear that zoom actions in the shortcut configuration window are related to the documentation view because generic zoom action UI names are placed in the KDevelop action group
  2. Future zoom action shortcut collisions are possible.
  3. Introduces changes to otherwise untouched documentationview, documentationcontroller.
  4. More complicated code structure including qobject_cast, which breaks encapsulation.

Maybe we don't really need to be able to configure these shortcuts...
For example all other documentation view action don't have configurable shortcuts either.

D2902 does not fix the original issue for me because I want larger font size for documentation, and my system default is too small.
Maybe it works well in Plasma, but not in XFCE and possibly some other environments.
Being able to increase/decrease documentation scale can be a useful feature in many use-cases.

This implementation does not clutter the UI and does not complicate the existing documentation view code much.
I think that this review request is an improvement over D774 and https://git.reviewboard.kde.org/r/126685/

ZoomController and ZoomActions utilities can be used to easily implement similar functionality in other areas.

apol added a subscriber: apol.Oct 12 2016, 11:08 PM

Wouldn't it be better just to allow the documentation view to change size with ctrl+mouse wheel or ctrl+'+'?

I really don't see why we need all of this.

In D3040#56430, @apol wrote:

Wouldn't it be better just to allow the documentation view to change size with ctrl+mouse wheel or ctrl+'+'?

I really don't see why we need all of this.

If context menu actions are undesirable, I can drop the last two commits from this review request and the last commit from D3041.
The first three D3040 commits and the first D3041 commit implement ctrl+mouse wheel zoom.
In this case there won't be any easy way to reset zoom level to the default value.

Eliminate documentation zoom context menu actions

  • Revert "Show zoom actions in StandardDocumentationView context menu"
  • Revert "Add ZoomActions utility"
apol added a comment.Oct 17 2016, 7:51 AM

Why did you create the ZoomController class instead of implementing everything locally in StandardDocumentationView?

ZoomController class serves two purposes:

  1. Separates generic scaling/zoom logic from the unrelated StandardDocumentationView logic. If I had placed zoom logic (complete with storing to KConfig) into StandardDocumentationView.cpp, it would occupy more than a half of that file!
  2. ZoomController class can be easily reused in another similar situation instead of duplicating generic scaling/storing logic.

Note that I preserved now unused zoomIn(), zoomOut() and resetZoom() slots in ZoomController.
This allows to add zoom actions in the future (either to documentation view or to another potential ZoomController-using code) without changes to ZoomController class.

mwolff requested changes to this revision.Feb 18 2017, 10:33 PM
mwolff added a subscriber: mwolff.

I think the zoomcontroller is OK, we could probably use that code elsewhere

CMakeLists.txt
7

unrelated

documentation/standarddocumentationview.cpp
43–44

put on its own line

Foo::Foo()
    : Base()
    , m_asdf(...)
{
    ...
}
util/zoomcontroller.cpp
24

newline before

25

newline before

27

newline before

30

newline before

i.e. we want to group includes

#include "file.h"

#include "..."

#include <K...>
#include <K...>

#include <Q...>
#include <Q...>

#include <c...>
util/zoomcontroller.h
45

please pass KConfigGroup directly

This revision now requires changes to proceed.Feb 18 2017, 10:33 PM
rjvbb added a subscriber: rjvbb.Feb 18 2017, 10:44 PM
In D3040#56430, @apol wrote:

Wouldn't it be better just to allow the documentation view to change size with ctrl+mouse wheel or ctrl+'+'?

Please don't hardcode ctrl+wheel zoom actions without either making it possible to disable the zoom feature or at the very least provide a "zoom to 100%" shortcut. Mouse wheel events are also generated when using (inertial) scrolling, and it is thus very easy to trigger zooming when you actually want to trigger a shortcut and scroll events are still coming in ("coasting").

Good point rene, let's use the quasi-standard Ctrl + 0 for that.

In D3040#87488, @mwolff wrote:

Good point rene, let's use the quasi-standard Ctrl + 0 for that.

++, would be nice to have in the editor views too.

I think I already mentioned on another ticket (or ML thread) that I've done some preliminary work on an implementation that protects against unwanted/opportunistic zooming?
https://github.com/RJVB/qtwheeltest

igorkushnir edited edge metadata.

Address review comments

  • Fix StandardDocumentationView code style
  • Pass KConfigGroup instead of config names to ZoomController
  • Reset documentation zoom on Ctrl+0

The downside of this simple Ctrl+0 implementation is that
StandardDocumentationView must have focus for it to work.
One has to click on the documentation widget, then press Ctrl+0.

The alternative "shortcut" implementation placed 3 zoom actions
(In, Out, Reset) in the documentation widget context menu.
This approach required the user to right click on the documentation
widget and select the desirable action from the context menu.
I have reverted commits that implemented these context menu actions
because @apol had asked to simplify the implementation.

There is also a patch that I attached in my comment to this review
request on the 12th of October. It is based on the now reverted
ZoomActions commits and allows assigning custom global KDevelop
shortcuts to the zoom actions. However this patch has more issues
than benefits as I described in the comment.

igorkushnir added inline comments.Feb 19 2017, 2:11 PM
CMakeLists.txt
7

I have changed StandardDocumentationView ABI. StandardDocumentationView is used by documentation plugins. So I thought that plugin version must be incremented.
Should this change be in a separate commit, separate review request or it's not needed at all?

igorkushnir marked 6 inline comments as done.Feb 19 2017, 2:13 PM
mwolff requested changes to this revision.Feb 20 2017, 9:13 PM
mwolff added inline comments.
CMakeLists.txt
7

still unrelated

documentation/standarddocumentationview.cpp
25

add a newline before

44

remove ()

63

default;

84

Q_ASSERT(!d->m_zoomController)

89

remove this context

125

shouldn't this also be handled in the zoom controller?

if (d->m_zoomController && d->m_zoomcontroller->handleKeyPressEvent(event)) {
    return;
}

like the code below

util/zoomcontroller.cpp
76

join next line

This revision now requires changes to proceed.Feb 20 2017, 9:13 PM
mwolff added inline comments.Feb 20 2017, 9:14 PM
CMakeLists.txt
7

sorry, just now do I see your comment. you can leave it in then

rjvbb added inline comments.Feb 20 2017, 9:26 PM
documentation/standarddocumentationview.cpp
84

Not my review, but FWIW, that's turning a function that only does something the 1st time into one that either crashes or else leaks ZoomController instances.

IMHO asserts like this should only be used for conditions that cannot be handled gracefully regardless of whether they should never happen.

igorkushnir marked 2 inline comments as done.Feb 21 2017, 7:26 PM

I guess everyone is OK with Ctrl+0 working only when the documentation widget has focus. This widget actually gets focus on scrolling, even though individual windows on my system don't get focus in such cases. So this should not be a problem.

I've noticed an issue with my own shortcut implementation: the shortcut does not work in the Programmer Dvorak layout, because this layout requires pressing Shift to access digits. It also does not work with numpad 0. So I'd like to replace event->modifiers() == Qt::ControlModifier with (event->modifiers() & Qt::ControlModifier). This will reset zoom level even if Ctrl+Shift+0 or Ctrl+Alt+0 is pressed. Please let me know if this is likely to cause conflicts with some global shortcuts. If it is, I could write even more specific code: (event->modifiers() & Qt::ControlModifier) && !(event->modifiers() & ~(Qt::ControlModifier | Qt::ShiftModifier | Qt::KeypadModifier)).

documentation/standarddocumentationview.cpp
84

This code mimics what QWidget::insertAction does with a nullptr parameter.

@mwolff, please confirm that you still prefer assertion. I'll make the change then.

rjvbb added a comment.Feb 21 2017, 7:46 PM

I've noticed an issue with my own shortcut implementation: the shortcut does not work in the Programmer Dvorak layout, because this layout requires pressing Shift to access digits. It also does not work with numpad 0. So I'd like to replace event->modifiers() == Qt::ControlModifier with (event->modifiers() & Qt::ControlModifier). This will reset zoom level even if Ctrl+Shift+0 or Ctrl+Alt+0 is pressed. Please let me know if this is likely to cause conflicts with some global shortcuts. If it is, I could write even more specific code: (event->modifiers() & Qt::ControlModifier) && !(event->modifiers() & ~(Qt::ControlModifier | Qt::ShiftModifier | Qt::KeypadModifier)).

Wouldn't it be possible to define an application-wide "Reset Zoom" (or "Zoom 100%") action with a shortcut that can be configured via the usual mechanism? That way you wouldn't have to jump through hoops in your code, and the action would (ultimately) become available in the other contexts where it would be most welcome.

In D3040#88399, @rjvbb wrote:

Wouldn't it be possible to define an application-wide "Reset Zoom" (or "Zoom 100%") action with a shortcut that can be configured via the usual mechanism? That way you wouldn't have to jump through hoops in your code, and the action would (ultimately) become available in the other contexts where it would be most welcome.

Will this action reset zoom in all KDevelop widgets? I don't think that this is desirable...
Or should every registered widget check if it has focus before resetting its zoom level?

rjvbb added a comment.Feb 21 2017, 8:50 PM

Will this action reset zoom in all KDevelop widgets? I don't think that this is desirable...
Or should every registered widget check if it has focus before resetting its zoom level?

I guess that depends on the exact way you implement it, but I would assume that it should be possible to do it in such a way that the action applies only to the focussed widget. After all, the "Close" action doesn't close all widgets either, just the one that has focus.

In D3040#88407, @rjvbb wrote:

I guess that depends on the exact way you implement it, but I would assume that it should be possible to do it in such a way that the action applies only to the focussed widget. After all, the "Close" action doesn't close all widgets either, just the one that has focus.

A possible implementation of this global action:

  1. Add a Reset Zoom action to actionCollection() in KDevelop::MainWindowPrivate::setupActions(). Assign a default shortcut Ctrl+0 to this new action.
  2. Optionally add this action to the KDevelop View menu.
  3. Register a custom event type resetZoomEvent and make its type code available to all KDevelop code globally.
  4. Send QEvent{resetZoomEvent} to QApplication::focusWidget() in a new slot connected to the Reset Zoom action.
  5. Override QObject::customEvent in StandardDocumentationView, propagate the event to ZoomController and accept it if its type matches the global resetZoomEvent type.
  6. It will be possible to add Reset Zoom support to other widgets simply by handling resetZoomEvent.

Issues:

  1. I'm not sure how best to make the custom event type globally accessible. Maybe a const member function can be added to KDevelop::MainWindow and accessed through ICore::self()->uiController()->activeMainWindow() (if it is not nullptr).
  2. I know little about KDevelop code and I'm not very experienced in Qt UI development, so this is most likely a suboptimal implementation plan.
  3. I doubt that this complex global action implementation fits well into this review request. This probably warrants a separate review.
igorkushnir edited edge metadata.
igorkushnir marked 8 inline comments as done.

Improve Ctrl+0 shortcut support, address review comments

I'm using a relatively safe and simple solution for now: allow (Qt::ControlModifier | Qt::ShiftModifier | Qt::KeypadModifier).

mwolff requested changes to this revision.Mar 5 2017, 7:39 PM

lgtm in principle, but there are two checks that imo need to be removed to cleanup the code. it's not a good idea to be overly pedantic in code, rather use assertions like we do elsewhere

util/zoomcontroller.cpp
109

this doesn't actually happen, right? Imo you can simply replace this with a Q_ASSERT, or remove it altogether

137

dito above

This revision now requires changes to proceed.Mar 5 2017, 7:39 PM
igorkushnir edited edge metadata.
  • Replace qCWarningS with Q_ASSERTs
igorkushnir marked 4 inline comments as done.Mar 8 2017, 12:36 PM
mwolff requested changes to this revision.Mar 19 2017, 1:04 PM

some minor nitpicks. imo feel free to commit after fixing those

CMakeLists.txt
7

unrelated change

documentation/standarddocumentationview.cpp
84

use Q_ASSERT_X

util/zoomcontroller.cpp
162

this shouldn't be required, or is it?

util/zoomcontroller.h
113

this should be called zoomFactorChanged

This revision now requires changes to proceed.Mar 19 2017, 1:04 PM
rjvbb added inline comments.Mar 19 2017, 3:02 PM
documentation/standarddocumentationview.cpp
84

So in production code it's OK just to leak a ZoomController, causing whatever side-effects because the previous instance was never deleted and thus still connected...?

igorkushnir edited edge metadata.
  • Minor cleanup
igorkushnir marked 2 inline comments as done.Mar 19 2017, 4:38 PM
igorkushnir added inline comments.
CMakeLists.txt
7

It's related. You have already written this comment, I've replied that I had changed StandardDocumentationView ABI, then you have agreed to leave this change in.

util/zoomcontroller.cpp
162

You are right, including the moc file is not needed. Removed.

util/zoomcontroller.h
113

This is a ZoomController class. It has a zoom factor, which is referred to as simply "factor" in member function names. It's obvious what factor it is as ZoomController shouldn't have any other factors. Do you want me to rename all 3 members: factor(), setFactor() and factorChanged()? What about d->m_factor and other variables in zoomcontroller.cpp?

mwolff accepted this revision.Mar 22 2017, 8:59 AM

remove the moc, then feel free to push

CMakeLists.txt
7

sorry :-/

util/zoomcontroller.h
113

leave it as is then, you are right - doesn't make much of a difference

This revision is now accepted and ready to land.Mar 22 2017, 8:59 AM
igorkushnir marked an inline comment as done.Mar 22 2017, 9:08 AM

This branch is based on a very old KDevPlatform revision. There are conflicts when rebasing on master. Should I rebase on master and update this review request? Or maybe rebase on some other branch?

kfunk added a subscriber: kfunk.Mar 22 2017, 9:17 AM

This branch is based on a very old KDevPlatform revision. There are conflicts when rebasing on master. Should I rebase on master and update this review request? Or maybe rebase on some other branch?

Rebase on master. And sorry that it tooks so long :)

Rebasing on master.

Works with QtWebKit and QtWebEngine. But I get crashes when clicking on the man pages documentation tree - with and without my changes. I don't know if something broke locally for me or this is a recent regression.

  • Add ZoomController utility
  • Implement Ctrl+mouse_scroll zoom in StandardDocumentationView
  • Reset documentation zoom on Ctrl+0

still lgtm, do you have commit rights?

aacid added a subscriber: aacid.Mar 27 2017, 10:31 PM

He does not have commit rights

This revision was automatically updated to reflect the committed changes.