Skip to content

feat(search): revive unified search - #10639

Open
Rello wants to merge 3 commits into
masterfrom
feature/newSearch
Open

feat(search): revive unified search#10639
Rello wants to merge 3 commits into
masterfrom
feature/newSearch

Conversation

@Rello

@Rello Rello commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator
  • Modernize the account-scoped search window with provider, date, and people filters, connected-service results, and aggregate and provider detail views.
  • Introduce cancellable per-provider search state, stable reveal ordering, stale-response protection, pagination, scoped retries, and persistent keyboard selection.
  • Align the search UI with shared wizard styling and add coverage for the search models, QML behavior, people lookup, and image handling.

Server Reference: nextcloud/server#60241

Assisted-by: Codex:GPT-5

Bildschirmfoto 2026-08-21 um 11 21 09
Bildschirmaufnahme.2026-08-21.um.11.22.02.mov

@Rello Rello added this to the 35.0.0 milestone Aug 21, 2026
@Rello Rello self-assigned this Aug 21, 2026
@Rello Rello added the design Design, UI, UX, etc. label Aug 21, 2026
@Rello

Rello commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

@kra-mo as discussed

@kra-mo kra-mo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is already a fixed-height dialog, there is no reason to show a filter button in the initial state. Instead, the filters should just always be visible.

There should also be some indication that a search is happening through a circular progress indicator like on the web. Right now, the search button seems to disappear while you're searching?

And are you struggling to click some of the buttons in the screen recording? I guess that is known, then :)

@kra-mo

kra-mo commented Aug 21, 2026

Copy link
Copy Markdown
Member

Since this is already a fixed-height dialog, there is no reason to show a filter button in the initial state. Instead, the filters should just always be visible.

Alternatively, we could make it variable-height, like Spotlight, which might also be nice.

@Rello

Rello commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Since this is already a fixed-height dialog, there is no reason to show a filter button in the initial state. Instead, the filters should just always be visible.

Alternatively, we could make it variable-height, like Spotlight, which might also be nice.

Hi,
you mean type/date/people buttons should be there from the beginning, correct?
I would like to keep it fixed height because this is stable. trying with dynamic and max-heigt is not used yet. Would add some more complexity. now we have the standard for all

@kra-mo

kra-mo commented Aug 21, 2026

Copy link
Copy Markdown
Member

Hi,
you mean type/date/people buttons should be there from the beginning, correct?

Yes, instead of having a funnel button.

I would like to keep it fixed height because this is stable. trying with dynamic and max-heigt is not used yet. Would add some more complexity. now we have the standard for all

Sure, makes sense

@Rello

Rello commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator Author

@kra-mo should be fine now

Bildschirmaufnahme.2026-08-22.um.16.11.20.mov

@Rello
Rello force-pushed the feature/newSearch branch from 69c9e0a to 5cfeaea Compare August 22, 2026 14:13
@Rello
Rello marked this pull request as ready for review August 22, 2026 14:13

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5cfeaeaac6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +38 to +39
else if (event.key === Qt.Key_Home) moveSelection(UnifiedSearchResultsListModel.First)
else if (event.key === Qt.Key_End) moveSelection(UnifiedSearchResultsListModel.Last)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve Home and End for editing the query

When the search field contains text, these handlers consume every Home/End key press (including Shift+Home/End) and mark the event accepted, so users can no longer move or extend the text cursor to the beginning or end of the query. Reserve these shortcuts for result navigation only under an explicit modifier or when focus is in the results list.

Useful? React with 👍 / 👎.

@Rello
Rello force-pushed the feature/newSearch branch 4 times, most recently from 860dcd2 to 7920816 Compare August 22, 2026 18:22

@kra-mo kra-mo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rello the 🔍 icon still disappears and there is still no progress indication.

@Rello

Rello commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

@Rello the 🔍 icon still disappears and there is still no progress indication.

Ah. I think it tries to put the progress indicator where the search icon is.
question is where to put it? always at the bottom of all results? ...because of the "load more" for example...

@kra-mo

kra-mo commented Aug 25, 2026

Copy link
Copy Markdown
Member

Where it is on the web. At the end of the bar, just before the X button:

image

@Rello

Rello commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

Where it is on the web. At the end of the bar, just before the X button:

Hi,
final approval like this?

Bildschirmfoto 2026-08-25 um 13 40 29

@kra-mo kra-mo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. It's an improvement over what there is atm.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08df11544b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

auto *const job = new JsonApiJob(account, QStringLiteral("ocs/v2.php/apps/files_sharing/api/v1/sharees"));
QUrlQuery query;
query.addQueryItem(QStringLiteral("search"), _searchTerm);
query.addQueryItem(QStringLiteral("shareType"), QStringLiteral("0"));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Send itemType with people lookups

For every non-empty People query, this bypasses the repository's OcsShareeJob::getSharees contract and omits the itemType parameter that the existing wrapper always adds and ShareeModel::fetch() supplies as either file or folder. Servers that enforce that Sharee API parameter will reject the request, leaving the new People picker in its error state; add an appropriate itemType and make the fake endpoint test reject requests that omit it so the real failure path is covered.

AGENTS.md reference: AGENTS.md:L33-L35

Useful? React with 👍 / 👎.

@Rello

Rello commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator Author

@copilot resolve the merge conflicts in this pull request

Copilot AI commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request

Resolved the merge conflicts and pushed the merge commit in 5d490ea.

Rello added 3 commits August 25, 2026 15:20
Modernize the account-scoped search window with provider, date, and
people filters, connected-service results, and aggregate and provider
detail views.

Introduce cancellable per-provider search state, stable reveal ordering,
stale-response protection, pagination, scoped retries, and persistent
keyboard selection.

Align the search UI with shared wizard styling and add coverage for the
search models, QML behavior, people lookup, and image handling.

Signed-off-by: Rello <github@scherello.de>
Assisted-by: Codex:GPT-5
Assisted-by: Codex:GPT-5
Signed-off-by: Rello <github@scherello.de>
Show the loading indicator on the trailing side of the search field without replacing the permanent search icon.

Signed-off-by: Rello <github@scherello.de>
Assisted-by: Codex:GPT-5

QVariant UnifiedSearchPeopleModel::data(const QModelIndex &index, int role) const
{
if (!checkIndex(index, CheckIndexOption::IndexIsValid)) return {};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!checkIndex(index, CheckIndexOption::IndexIsValid)) return {};
Q_ASSERT(!checkIndex(index, CheckIndexOption::IndexIsValid));

if (_job) {
disconnect(_job, nullptr, this, nullptr);
if (const auto job = qobject_cast<JsonApiJob *>(_job.data()); job && job->reply() && job->reply()->isRunning()) job->reply()->abort();
_job->deleteLater();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume the job to delete itself
I think this is not crashing because:

  • the job had time to delete itself and the QPointer would have been null
  • the job is still valid and 2 calls to deleteLater are not causing 2 deletions

[[nodiscard]] QString searchTerm() const;
[[nodiscard]] bool busy() const;
[[nodiscard]] QString errorString() const;
Q_INVOKABLE void retry();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be a slot

Comment on lines +120 to +123
explicit UnifiedSearchResultsListModel(AccountState *accountState,
QObject *parent = nullptr,
int debounceInterval = 300,
int revealInterval = 1000);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

usually we keep QObject *parent parameter the last one
that is a convention to follow

Suggested change
explicit UnifiedSearchResultsListModel(AccountState *accountState,
QObject *parent = nullptr,
int debounceInterval = 300,
int revealInterval = 1000);
explicit UnifiedSearchResultsListModel(AccountState *accountState,
int debounceInterval = 300,
int revealInterval = 1000,
QObject *parent = nullptr);

Comment on lines +257 to +259
if (!checkIndex(index, QAbstractItemModel::CheckIndexOption::IndexIsValid)) {
return {};
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!checkIndex(index, QAbstractItemModel::CheckIndexOption::IndexIsValid)) {
return {};
}
Q_ASSERT(!checkIndex(index, QAbstractItemModel::CheckIndexOption::IndexIsValid));

if (const auto job = qobject_cast<JsonApiJob *>(jobObject.data()); job && job->reply() && job->reply()->isRunning()) {
job->reply()->abort();
}
jobObject->deleteLater();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msut be removed
our jobs self delete when they are finished

if (const auto job = qobject_cast<JsonApiJob *>(_providerDiscoveryJob.data()); job && job->reply() && job->reply()->isRunning()) {
job->reply()->abort();
}
_providerDiscoveryJob->deleteLater();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must be removed
our JsonApiJob class will delete itself

import QtQuick
import QtQuick.Controls.Basic as BasicControls
import QtQuick.Layouts
import Qt5Compat.GraphicalEffects

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be used as this is deprecated and only provided for porting apps from Qt5 to Qt6

import QtQuick.Controls
import QtQuick.Controls.Basic as BasicControls
import QtQuick.Layouts
import Qt5Compat.GraphicalEffects

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should not be used as this is deprecated and only provided for porting apps from Qt5 to Qt6

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
78.4% Coverage on New Code (required ≥ 80%)
99 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@github-actions

Copy link
Copy Markdown
Contributor

Artifact containing the AppImage: nextcloud-appimage-pr-10639.zip

Digest: sha256:a2afe3438eb87defa2cfdf344040f3257fbfcdb442dfb5e01bf9abcb1d07f591

To test this change/fix you can download the above artifact file, unzip it, and run it.

Please make sure to quit your existing Nextcloud app and backup your data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

design Design, UI, UX, etc.

Projects

Status: NC35

Development

Successfully merging this pull request may close these issues.

4 participants