Differential D3040 Diff 11502 documentation/standarddocumentationview.cpp

Changeset View

Standalone View

View Options

documentation/standarddocumentationview.cpp

1/* 1​/*
2 * This file is part of KDevelop 2​ * This file is part of KDevelop
3 * Copyright 2010 Aleix Pol Gonzalez <aleixpol@kde.org> 3​ * Copyright 2010 Aleix Pol Gonzalez <aleixpol@kde.org>
4​ * Copyright 2016 Igor Kushnir <igorkuo@gmail.com>
4 * 5​ *
5 * This program is free software; you can redistribute it and/or modify 6​ * This program is free software; you can redistribute it and/or modify
6 * it under the terms of the GNU Library General Public License as 7​ * it under the terms of the GNU Library General Public License as
7 * published by the Free Software Foundation; either version 2 of the 8​ * published by the Free Software Foundation; either version 2 of the
8 * License, or (at your option) any later version. 9​ * License, or (at your option) any later version.
9 * 10​ *
10 * This program is distributed in the hope that it will be useful, 11​ * This program is distributed in the hope that it will be useful,
11 * but WITHOUT ANY WARRANTY; without even the implied warranty of 12​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
12 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the 13​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13 * GNU General Public License for more details. 14​ * GNU General Public License for more details.
14 * 15​ *
15 * You should have received a copy of the GNU General Public 16​ * You should have received a copy of the GNU General Public
16 * License along with this program; if not, write to the 17​ * License along with this program; if not, write to the
17 * Free Software Foundation, Inc., 18​ * Free Software Foundation, Inc.,
18 * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. 19​ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19 */ 20​ */
20 21​
21#include "standarddocumentationview.h" 22​#include "standarddocumentationview.h"
22#include "documentationfindwidget.h" 23​#include "documentationfindwidget.h"
23#include "debug.h" 24​#include "debug.h"
25​#include <util/zoomcontroller.h>
mwolffUnsubmitted
Done

add a newline before

mwolff: add a newline before
26​
27​#include <KConfigGroup>
28​#include <KSharedConfig>
24 29​
25#include <QFontDatabase> 30​#include <QFontDatabase>
31​#include <QKeyEvent>
26 32​
27using namespace KDevelop; 33​using namespace KDevelop;
28 34​
35​class KDevelop::StandardDocumentationViewPrivate
36​{
37​public:
38​ ZoomController* m_zoomController = nullptr;
39​ IDocumentation::Ptr m_doc;
40​};
41​
29StandardDocumentationView::StandardDocumentationView(DocumentationFindWidget* findWidget, QWidget* parent) 42​StandardDocumentationView::StandardDocumentationView(DocumentationFindWidget* findWidget, QWidget* parent)
30 : QWebView (parent) 43​ : QWebView(parent)
44​ , d{new StandardDocumentationViewPrivate()}
mwolffUnsubmitted
Done

put on its own line

Foo::Foo()
    : Base()
    , m_asdf(...)
{
    ...
}
mwolff: put on its own line ``` Foo::Foo() : Base() , m_asdf(...) { ... } ```
mwolffUnsubmitted
Done

remove ()

mwolff: remove `()`
31{ 45​{
32 findWidget->setEnabled(true); 46​ findWidget->setEnabled(true);
33 connect(findWidget, &DocumentationFindWidget::newSearch, this, &StandardDocumentationView::search); 47​ connect(findWidget, &DocumentationFindWidget::newSearch, this, &StandardDocumentationView::search);
34 48​
35 QFont sansSerifFont = QFontDatabase::systemFont(QFontDatabase::GeneralFont); 49​ QFont sansSerifFont = QFontDatabase::systemFont(QFontDatabase::GeneralFont);
36 QFont monospaceFont = QFontDatabase::systemFont(QFontDatabase::FixedFont); 50​ QFont monospaceFont = QFontDatabase::systemFont(QFontDatabase::FixedFont);
37 QFont minimalFont = QFontDatabase::systemFont(QFontDatabase::SmallestReadableFont); ​
38 51​
39 QWebSettings* s = settings(); 52​ QWebSettings* s = settings();
40 53​
41 s->setFontFamily(QWebSettings::StandardFont, sansSerifFont.family()); 54​ s->setFontFamily(QWebSettings::StandardFont, sansSerifFont.family());
42 s->setFontFamily(QWebSettings::SerifFont, "Serif"); 55​ s->setFontFamily(QWebSettings::SerifFont, "Serif");
43 s->setFontFamily(QWebSettings::SansSerifFont, sansSerifFont.family()); 56​ s->setFontFamily(QWebSettings::SansSerifFont, sansSerifFont.family());
44 s->setFontFamily(QWebSettings::FixedFont, monospaceFont.family()); 57​ s->setFontFamily(QWebSettings::FixedFont, monospaceFont.family());
45 58​
46 s->setFontSize(QWebSettings::DefaultFontSize, QFontInfo(sansSerifFont).pixelSize()); 59​ s->setFontSize(QWebSettings::DefaultFontSize, QFontInfo(sansSerifFont).pixelSize());
47 s->setFontSize(QWebSettings::DefaultFixedFontSize, QFontInfo(monospaceFont).pixelSize()); 60​ s->setFontSize(QWebSettings::DefaultFixedFontSize, QFontInfo(monospaceFont).pixelSize());
48 s->setFontSize(QWebSettings::MinimumFontSize, QFontInfo(minimalFont).pixelSize()); 61​}
62​
63​StandardDocumentationView::~StandardDocumentationView()
mwolffUnsubmitted
Done

default;

mwolff: = default;
64​{
49} 65​}
50 66​
51void StandardDocumentationView::search ( const QString& text, DocumentationFindWidget::FindOptions options ) 67​void StandardDocumentationView::search ( const QString& text, DocumentationFindWidget::FindOptions options )
52{ 68​{
53 //Highlighting has been commented because it doesn't let me jump around all occurrences 69​ //Highlighting has been commented because it doesn't let me jump around all occurrences
54// page()->findText(QString(), QWebPage::HighlightAllOccurrences); 70​// page()->findText(QString(), QWebPage::HighlightAllOccurrences);
55 71​
56 QWebPage::FindFlags ff=QWebPage::FindWrapsAroundDocument /*| QWebPage::HighlightAllOccurrences*/; 72​ QWebPage::FindFlags ff=QWebPage::FindWrapsAroundDocument /*| QWebPage::HighlightAllOccurrences*/;
57 if(options & DocumentationFindWidget::Previous) 73​ if(options & DocumentationFindWidget::Previous)
58 ff |= QWebPage::FindBackward; 74​ ff |= QWebPage::FindBackward;
59 75​
60 if(options & DocumentationFindWidget::MatchCase) 76​ if(options & DocumentationFindWidget::MatchCase)
61 ff |= QWebPage::FindCaseSensitively; 77​ ff |= QWebPage::FindCaseSensitively;
62 78​
63 page()->findText(text, ff); 79​ page()->findText(text, ff);
64} 80​}
65 81​
82​void StandardDocumentationView::initZoom(const QString& configSubGroup)
83​{
84​ if (Q_UNLIKELY(d->m_zoomController)) {
mwolffUnsubmitted
Done

Q_ASSERT(!d->m_zoomController)

mwolff: Q_ASSERT(!d->m_zoomController)
rjvbbUnsubmitted
Not Done

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.

rjvbb: Not my review, but FWIW, that's turning a function that only does something the 1st time into…
igorkushnirAuthorUnsubmitted
Done

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.

igorkushnir: This code mimics what [[ https://code.woboq.org/qt5/qtbase/src/widgets/kernel/qwidget.cpp.
mwolffUnsubmitted
Done

use Q_ASSERT_X

mwolff: use `Q_ASSERT_X`
rjvbbUnsubmitted
Not Done

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...?

rjvbb: So in production code it's OK just to leak a ZoomController, causing whatever side-effects…
85​ qCWarning(DOCUMENTATION) << "Calling StandardDocumentationView::initZoom a second time has no effect.";
86​ return;
87​ }
88​
89​ {
mwolffUnsubmitted
Done

remove this context

mwolff: remove this context
90​ const KConfigGroup outerGroup(KSharedConfig::openConfig(), QStringLiteral("Documentation View"));
91​ const KConfigGroup cg(&outerGroup, configSubGroup);
92​ d->m_zoomController = new ZoomController(cg, this);
93​ }
94​ connect(d->m_zoomController, &ZoomController::factorChanged,
95​ this, &StandardDocumentationView::updateZoomFactor);
96​ updateZoomFactor(d->m_zoomController->factor());
97​}
98​
66void StandardDocumentationView::setDocumentation(const IDocumentation::Ptr& doc) 99​void StandardDocumentationView::setDocumentation(const IDocumentation::Ptr& doc)
67{ 100​{
68 if(m_doc) 101​ if(d->m_doc)
69 disconnect(m_doc.data()); 102​ disconnect(d->m_doc.data());
70 m_doc = doc; 103​ d->m_doc = doc;
71 update(); 104​ update();
72 if(m_doc) 105​ if(d->m_doc)
73 connect(m_doc.data(), &IDocumentation::descriptionChanged, this, &StandardDocumentationView::update); 106​ connect(d->m_doc.data(), &IDocumentation::descriptionChanged, this, &StandardDocumentationView::update);
74} 107​}
75 108​
76void StandardDocumentationView::update() 109​void StandardDocumentationView::update()
77{ 110​{
78 if(m_doc) 111​ if(d->m_doc)
79 setHtml(m_doc->description()); 112​ setHtml(d->m_doc->description());
80 else 113​ else
81 qCDebug(DOCUMENTATION) << "calling StandardDocumentationView::update() on an uninitialized view"; 114​ qCDebug(DOCUMENTATION) << "calling StandardDocumentationView::update() on an uninitialized view";
82} 115​}
116​
117​
118​void StandardDocumentationView::updateZoomFactor(double zoomFactor)
119​{
120​ setZoomFactor(zoomFactor);
121​}
122​
123​void StandardDocumentationView::keyPressEvent(QKeyEvent* event)
124​{
125​ if (d->m_zoomController && event->modifiers() == Qt::ControlModifier && event->key() == Qt::Key_0) {
mwolffUnsubmitted
Done

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

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

like the code below

mwolff: shouldn't this also be handled in the zoom controller? ``` if (d->m_zoomController && d…
126​ d->m_zoomController->resetZoom();
127​ event->accept();
128​ return;
129​ }
130​ QWebView::keyPressEvent(event);
131​}
132​
133​void StandardDocumentationView::wheelEvent(QWheelEvent* event)
134​{
135​ if (d->m_zoomController && d->m_zoomController->handleWheelEvent(event))
136​ return;
137​ QWebView::wheelEvent(event);
138​}