Commit e929d4c1 authored by Harald Sitter's avatar Harald Sitter 🏳️‍🌈
Browse files

unbreak button states

versionpage used KAssitantDialog's setAppropriate but that triggers
internal re-evaluation of button states as per the appropriateness of
adjacent pages of the current page.
this then would short circuit when a premature conclusion page was shown
(because the user provided no or insufficient information to file a
report) and result in the back button getting enabled even when that
wasn't reasonable (i.e. finished was emitted with showBack=false).

notably this broke for none-kde addresses that aren't routed into
bugzilla iff bugzilla responses are so slow that they'd arrive after the
user reached the conclusion page

the fix for this is super awkward. instead of using the existing
appropriateness system, use a higher level one on our end. this also
requires us to be able to iterate the page sequence. to achieve that we
now put all pages in a vector in the sequence that they are in the
pagedialog. on page changes we then establish whether we were moving
forward or backward and this way skip over inappropriate pages.

(could have used the hash, turn it into a map and use that instead, of a
new vector but honestly I want the hash to go away and we'll manage the
hand full of extra bytes until then)
parent 4a5270c7
/*******************************************************************
* SPDX-FileCopyrightText: 2019 Harald Sitter <sitter@kde.org>
* SPDX-FileCopyrightText: 2019-2021 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-or-later
******************************************************************/
......@@ -29,7 +29,7 @@ BugzillaVersionPage::BugzillaVersionPage(ReportAssistantDialog *parent)
connect(bugzillaManager(), &BugzillaManager::bugzillaVersionFound, this, [=] {
// Don't show this page ever again!
assistant()->setAppropriate(m_item, false);
appropriate = false;
if (assistant()->currentPage() == m_item) {
assistant()->next();
}
......@@ -59,6 +59,11 @@ bool BugzillaVersionPage::isComplete()
return false;
}
bool BugzillaVersionPage::isAppropriate()
{
return appropriate;
}
KPageWidgetItem *BugzillaVersionPage::item() const
{
return m_item;
......
/*******************************************************************
* SPDX-FileCopyrightText: 2019 Harald Sitter <sitter@kde.org>
* SPDX-FileCopyrightText: 2019-2021 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-or-later
******************************************************************/
......@@ -27,10 +27,12 @@ public:
KPageWidgetItem *item() const;
virtual bool isComplete() override;
virtual bool isAppropriate() override;
private:
Ui::BugzillaVersionPage *ui = nullptr;
KPageWidgetItem *m_item = nullptr;
bool appropriate = true;
};
#endif // ASSISTANTPAGE_BUGZILLA_VERSION_H
......@@ -16,6 +16,7 @@
#include <KMessageBox>
#include "drkonqi.h"
#include "drkonqi_debug.h"
#include "backtracegenerator.h"
#include "debuggermanager.h"
......@@ -59,8 +60,7 @@ ReportAssistantDialog::ReportAssistantDialog(QWidget *parent)
m_pageWidgetMap.insert(QLatin1String(PAGE_INTRODUCTION_ID), m_introductionPage);
m_introductionPage->setHeader(i18nc("@title", "Welcome to the Reporting Assistant"));
m_introductionPage->setIcon(QIcon::fromTheme(QStringLiteral("tools-report-bug")));
addPage(m_introductionPage);
m_pageItems.push_back(m_introductionPage);
}
//-Bug Awareness Page
......@@ -140,16 +140,21 @@ ReportAssistantDialog::ReportAssistantDialog(QWidget *parent)
m_bugzillaSendPage->setIcon(QIcon::fromTheme(QStringLiteral("applications-internet")));
connect(m_bugzillaSend, &BugzillaSendPage::finished, this, &ReportAssistantDialog::assistantFinished);
// TODO Remember to keep the pages ordered
addPage(m_awarenessPage);
addPage(m_backtracePage);
addPage(m_conclusionsPage);
addPage(versionPage->item());
addPage(m_bugzillaLoginPage);
addPage(m_bugzillaDuplicatesPage);
addPage(m_bugzillaInformationPage);
addPage(m_bugzillaPreviewPage);
addPage(m_bugzillaSendPage);
// Need to append because the welcome page is conditionally pushed early on.
m_pageItems.insert(m_pageItems.end(),
{m_awarenessPage,
m_backtracePage,
m_conclusionsPage,
versionPage->item(),
m_bugzillaLoginPage,
m_bugzillaDuplicatesPage,
m_bugzillaInformationPage,
m_bugzillaPreviewPage,
m_bugzillaSendPage});
for (const auto pageItem : m_pageItems) {
addPage(pageItem);
}
// Force a 16:9 ratio for nice appearance by default.
QSize aspect(16, 9);
......@@ -197,6 +202,57 @@ void ReportAssistantDialog::currentPageChanged_slot(KPageWidgetItem *current, KP
// Load data of the current(new) page
if (current) {
ReportAssistantPage *currentPage = qobject_cast<ReportAssistantPage *>(current->widget());
if (!currentPage->isAppropriate()) {
// The page is inappropriate. Find where to go next. This is extra exhausting because
// we can't just find the next or previous appropriate page because next() and back() implement lots of
// bespoke movement rules. So we in fact constantly need to go through those functions and then check again here.
// WARNING: when no page is appropriate this produces an ininfinite loop. There is no way to
// prevent this right now. next() and back() would need to stop hacking in shortcuts for us to reliably
// determine where to go and when we can't go anywhere.
enum class Movement { Unknown, Forward, Backward };
auto move = Movement::Unknown;
for (auto it = m_pageItems.cbegin(); it != m_pageItems.cend(); ++it) {
if (*it == before && move == Movement::Unknown) {
// Found previous item first, we are moving forward ->
move = Movement::Forward;
continue; // continue finding current item, depending on where it is in the container we may need to reverse
}
if (*it == current) {
if (move == Movement::Unknown) {
// Current item first, we are moving backward <-
move = Movement::Backward;
if (it == m_pageItems.cbegin()) {
// Turn around if this item is first but inappropriate.
move = Movement::Forward;
}
} else if ((it + 1) == m_pageItems.cend()) {
// Turn around if this item is last but inappropriate.
move = Movement::Backward;
}
break;
}
}
qCDebug(DRKONQI_LOG) << "page inappropriate, skipping";
switch (move) {
case Movement::Unknown:
qCDebug(DRKONQI_LOG) << "unknown";
Q_UNREACHABLE();
Q_FALLTHROUGH(); // do whatever at this point but do something, the page is inappropriate.
case Movement::Forward:
qCDebug(DRKONQI_LOG) << "forward";
next();
return;
case Movement::Backward:
qCDebug(DRKONQI_LOG) << "backward";
back();
return;
}
return;
}
nextButton()->setEnabled(currentPage->isComplete());
currentPage->aboutToShow();
......
/*******************************************************************
* reportassistantdialog.h
* SPDX-FileCopyrightText: 2009 Dario Andres Rodriguez <andresbajotierra@gmail.com>
* SPDX-FileCopyrightText: 2019 Harald Sitter <sitter@kde.org>
* SPDX-FileCopyrightText: 2019-2021 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-or-later
*
......@@ -66,6 +66,8 @@ private:
QIcon m_nextButtonIconCache;
QString m_nextButtonTextCache;
// Page sequence.
std::vector<KPageWidgetItem *> m_pageItems;
};
#endif
/*******************************************************************
* reportassistantpage.cpp
* SPDX-FileCopyrightText: 2009 Dario Andres Rodriguez <andresbajotierra@gmail.com>
* SPDX-FileCopyrightText: 2021 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-or-later
*
......@@ -20,6 +21,11 @@ bool ReportAssistantPage::isComplete()
return true;
}
bool ReportAssistantPage::isAppropriate()
{
return true;
}
bool ReportAssistantPage::showNextPage()
{
return true;
......
/*******************************************************************
* reportassistantpage.h
* SPDX-FileCopyrightText: 2009 Dario Andres Rodriguez <andresbajotierra@gmail.com>
* SPDX-FileCopyrightText: 2021 Harald Sitter <sitter@kde.org>
*
* SPDX-License-Identifier: GPL-2.0-or-later
*
......@@ -35,6 +36,13 @@ public:
/** Tells the KAssistantDialog to enable the Next button **/
virtual bool isComplete();
/**
* Whether this page is fit for displaying. An inappropriate page is skipped.
* This replicates KAssistantDialog's appropriateness system as it'd mess with our (manual) button state management.
* An in appropriate page will be skipped.
*/
virtual bool isAppropriate();
/** Last time checks to see if you can turn the page **/
virtual bool showNextPage();
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment