From 8743f4828d8ce31879b56338c284b72757530548 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 24 Aug 2026 18:23:50 +0200 Subject: feat(compose): offer Edit on a draft in the message pane's bar Item 157, the half item 153 did not close. A draft was editable by double-click and by a Message-menu entry, neither of which is where the user looks while reading one. populateMessageBar() swaps the reply pair for edit_draft on a displayed draft. Three things came out of hand-testing it, each invisible to the tests written before them. The bar keyed on currentIndex(), which a query leaves valid on a row of the discarded result, so it kept the draft button after switching to the inbox and the reply pair after switching to drafts. This is item 150's trap one level up. It answers from m_currentMessageId/m_currentThreadId now, which every blanking route clears, refilled from showPlaceholderPane(), the one site all five of those routes share. That exposed a defect predating the bar: updateComposeActions() ran only from the two selection handlers, so Reply and Forward stayed enabled over a blank pane. Invisible while they sat on the main toolbar among always-on actions. The bar is hidden over an empty pane, so it comes and goes with the subject and the details button rather than hovering over the logo. That in turn broke the showing half: setBarActions() runs before showThread() fills m_items, so the first message opened after a blanking left the bar hidden and the second showed it from stale items, one selection behind for the life of the view. updateHeader() shows it, beside the details button it rides with. The test missed the last one by asserting before the render landed, measuring the placeholder; it waits on showingPlaceholder() now. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KEcn3u19xPqv6ggD15PG4c --- src/mainwindow.cpp | 96 +++++++++++++++++++++++++++++++++++++++++++++++++---- src/messageview.cpp | 22 +++++++++++- 2 files changed, 110 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index eb41da9..42fe1bf 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1298,7 +1298,17 @@ void MainWindow::updateComposeActions() // cannot send. save_message is deliberately NOT in this list: it is the // escape hatch for exactly that case, writing the raw message to a file // that can be attached to a new message from an account that can send. - const QString replyAccount = accountForCurrentMessage(); + // Everything below describes the DISPLAYED message, and every route that + // blanks the pane clears these two ids while leaving currentIndex() valid + // on a row from the previous result. Answering from the index instead + // would describe a message that is no longer on screen: it left Reply + // enabled over an empty pane, and, once this function refilled the pane's + // bar, put the wrong buttons there in both directions. + const bool showing = + !m_currentMessageId.isEmpty() || !m_currentThreadId.isEmpty(); + + const QString replyAccount = + showing ? accountForCurrentMessage() : QString(); const bool canReply = !replyAccount.isEmpty() && m_config.account(replyAccount).canSend(); @@ -1320,7 +1330,13 @@ void MainWindow::updateComposeActions() // notmuch surfaces the Maildir D flag as one, and a message flagged by // another client sits in the inbox rather than in the drafts folder. if (QAction *edit = m_actions.value(QStringLiteral("edit_draft"))) - edit->setEnabled(currentMessageIsADraft()); + edit->setEnabled(showing && currentMessageIsADraft()); + + // The pane's bar shows Edit draft in place of the reply pair on a draft + // (item 157), so it is refilled here rather than once at construction: + // this runs on every selection change, which is the only thing that can + // move a draft into or out of the pane. + populateMessageBar(); // The ribbon appears only when an account was identified AND it cannot // send. An unidentified account is not a receive-only one: it is a message @@ -2116,14 +2132,63 @@ void MainWindow::populateMessageBar() // Reply and Forward only: Compose needs no message and sits on the main // toolbar with the other window-wide actions. // + // A draft swaps that pair for Edit draft (item 157). It is the same rule + // items 139 to 141 settled, applied one level down: the bar carries what + // the DISPLAYED message affords, and a draft affords neither answering a + // sender it does not have nor passing on a message that is not finished. + // The view controls are unchanged by the swap, since how the pane renders + // is not a property of what the message is. + // + // Called from updateComposeActions() as well as at construction, so it + // follows the selection. That is also why the actions are looked up fresh + // rather than cached: the bar is refilled, never rebuilt. + // // Slightly smaller than the main toolbar's icons, deriving from the // configured size rather than hardcoding one, so the bar stays subordinate // to the chrome above it however the user sets that key. const int iconSize = qMax(16, (m_config.toolbarIconSize() * 7) / 8); + + // Gated on whether a message is DISPLAYED, not on which row is current. + // The two disagree on every route that blanks the pane without moving the + // selection: running a query leaves currentIndex() valid on a row from the + // previous result, so a bar keyed on it kept offering Edit draft over an + // empty pane after leaving the Drafts filter, and the reply pair after + // arriving at it. This is item 150's trap exactly, one level up, and the + // test that missed it moved row to row, which is the one gesture that + // cannot expose it. + // + // m_currentMessageId is cleared with the pane by every one of those + // routes, so it is the only thing that tracks what the bar describes. + // The bar always carries a message half. Whether it is SEEN is + // MessageView's question, not this one: it hides the whole bar over an + // empty pane, alongside the subject and the details button, so this only + // ever decides what a displayed message affords. + // + // Only a displayed DRAFT swaps the pair, and "displayed" is the operative + // word: keyed on m_currentMessageId rather than on currentIndex(), which + // stays valid on a row from the previous result after a query and made the + // bar describe a message that was no longer on screen, in both directions. + // That is item 150's trap one level up, and the first version of this test + // could not see it because it moved row to row, the one gesture that + // always changes both. + // currentMessageIsADraft() alone, with no displayed-message guard beside + // it: a query calls m_model->clear(), which invalidates currentIndex(), + // so the predicate is already false whenever the pane is blank. Measured, + // after writing that guard and finding no reachable state where it + // changed the answer. The guard that IS load-bearing sits one level up in + // updateComposeActions(), where accountForCurrentMessage() would otherwise + // answer about a row this query is discarding. + QList messageActions; + if (currentMessageIsADraft()) { + messageActions = { m_actions.value(QStringLiteral("edit_draft")) }; + } else { + messageActions = { m_actions.value(QStringLiteral("reply")), + m_actions.value(QStringLiteral("forward")) }; + } + m_messageView->setBarActions( - { m_actions.value(QStringLiteral("reply")), - m_actions.value(QStringLiteral("forward")) }, - { m_actions.value(QStringLiteral("toggle_html")) }, iconSize); + messageActions, { m_actions.value(QStringLiteral("toggle_html")) }, + iconSize); } void MainWindow::showShortcutReference() @@ -2566,6 +2631,22 @@ void MainWindow::showPlaceholderPane() { m_messageView->showPlaceholder(placeholderHelpers()); + // Every route that blanks the pane comes through here, which is why the + // bar is refilled here rather than at each of them: item 150 was the same + // defect one level down and was fixed by finding the one shared site. + // The ids this reads are cleared by the callers around the same point; + // the order between the two was measured and no test can tell it apart, + // since a query invalidates currentIndex() anyway. Left as it was found. + // + // updateComposeActions() rather than populateMessageBar() alone, because + // the ENABLEMENT was stale here too and had been since before the bar + // existed: it ran only from the two selection handlers, so a query that + // blanked the pane left Reply and Forward enabled over nothing. Invisible + // while they lived on the main toolbar among other always-on actions, and + // plain once they sat over an empty pane. It calls populateMessageBar() + // last, so the bar still gets refilled. + updateComposeActions(); + QMetaObject::invokeMethod(m_worker, "requestCounts", Qt::QueuedConnection, Q_ARG(QStringList, placeholderQueries()), Q_ARG(quint64, ++m_countsGeneration)); @@ -3171,8 +3252,6 @@ void MainWindow::runQuery(FlatResult flat, AccountScope scope) ++m_generation; m_model->clear(); - m_messageView->clear(); - showPlaceholderPane(); // Cleared WITH the pane, not merely alongside it. These three name what the // pane is showing, and both selection handlers use them to decide whether a @@ -3185,6 +3264,9 @@ void MainWindow::runQuery(FlatResult flat, AccountScope scope) // the thread is current at the moment the query replaces the view, and its // one card opens onto the placeholder. A query returning any OTHER thread // hides it, which is why it took a screenshot to find. + m_messageView->clear(); + showPlaceholderPane(); + m_currentThreadId.clear(); m_currentMessageId.clear(); m_currentMessageThreadId.clear(); diff --git a/src/messageview.cpp b/src/messageview.cpp index 4c2a9d0..86e40eb 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -617,7 +617,18 @@ void MessageView::setBarActions(const QList &messageActions, } } - m_messageBar->setVisible(!m_messageBar->actions().isEmpty()); + // Hidden over an empty pane whatever it holds, so it comes and goes with + // the subject and the details button rather than hovering over the logo. + // The user's call, and the reason is consistency with those two: a bar + // that persists was the only piece of header furniture that did. + // + // This guard covers the HIDING only. MainWindow refills the bar from + // showPlaceholderPane(), which every route that blanks the pane passes + // through, so the refill re-reads m_items and this line answers. Nothing + // refills it when a message ARRIVES, so updateHeader() shows it there; + // see the note beside the details button, which it rides with. + m_messageBar->setVisible(!m_messageBar->actions().isEmpty() + && !m_items.isEmpty()); } void MessageView::clear() @@ -754,6 +765,14 @@ void MessageView::updateHeader() // scratch a few lines down. m_headerOffers.clear(); + // The bar rides with the details button, but only the SHOWING half belongs + // here. Hiding is covered by setBarActions(), since MainWindow refills the + // bar on every route that blanks the pane, and a hide() in the empty + // branch below was measured to change nothing. Nothing refills the bar + // when a message ARRIVES, though, so without the show() below it stayed + // hidden for the first message opened after any blanking and appeared on + // the second, when m_items still held the first: one selection behind for + // as long as the view lasted. if (m_items.isEmpty()) { m_headerLabel->clear(); m_detailsButton->hide(); @@ -761,6 +780,7 @@ void MessageView::updateHeader() } m_detailsButton->show(); + m_messageBar->setVisible(!m_messageBar->actions().isEmpty()); // The thread's subject comes from its first message; later replies carry // Re: prefixes that add nothing. -- cgit v1.2.3