feat(search): revive unified search - #10639
Conversation
|
@kra-mo as discussed |
kra-mo
left a comment
kra-mo
left a comment
There was a problem hiding this comment.
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 :)
Alternatively, we could make it variable-height, like Spotlight, which might also be nice. |
Hi, |
Yes, instead of having a funnel button.
Sure, makes sense |
|
@kra-mo should be fine now Bildschirmaufnahme.2026-08-22.um.16.11.20.mov |
69c9e0a to
5cfeaea
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| else if (event.key === Qt.Key_Home) moveSelection(UnifiedSearchResultsListModel.First) | ||
| else if (event.key === Qt.Key_End) moveSelection(UnifiedSearchResultsListModel.Last) |
There was a problem hiding this comment.
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 👍 / 👎.
860dcd2 to
7920816
Compare
Ah. I think it tries to put the progress indicator where the search icon is. |
|
Where it is on the web. At the end of the bar, just before the X button: 
|
Hi, 
|
kra-mo
left a comment
kra-mo
left a comment
There was a problem hiding this comment.
Sure. It's an improvement over what there is atm.
There was a problem hiding this comment.
💡 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")); |
There was a problem hiding this comment.
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 👍 / 👎.
|
@copilot resolve the merge conflicts in this pull request |
5d490ea to
ba58f8b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cefa5a12d7
ℹ️ 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".
|
|
||
| QVariant UnifiedSearchResultsListModel::data(const QModelIndex &index, int role) const | ||
| { | ||
| Q_ASSERT(!checkIndex(index, QAbstractItemModel::CheckIndexOption::IndexIsValid)); |
There was a problem hiding this comment.
In builds with assertions enabled, every valid QML/model lookup aborts here: checkIndex(...IndexIsValid) returns true for a valid index, and the newly added ! therefore makes Q_ASSERT fail as soon as search results are displayed. The same inverted assertion was also introduced in UnifiedSearchPeopleModel::data() at src/gui/search/unifiedsearchpeoplemodel.cpp:35, so opening populated people results has the same failure mode.
Useful? React with 👍 / 👎.
d4d8159 to
f376c4e
Compare
|
| id: activityContent | ||
|
|
||
| adaptiveTextColor: root.activeFocus ? palette.highlightedText : palette.text | ||
| adaptiveTextColor: palette.text |
There was a problem hiding this comment.
Is this related to unified search changes?
| id: activityHover | ||
| anchors.fill: activityList.currentItem | ||
| color: palette.highlight | ||
| color: Style.listItemHoverBackground |
There was a problem hiding this comment.
Is this related to unified search changes?
| Item { | ||
| id: detailHeader | ||
|
|
||
| objectName: "searchDetailHeader" | ||
| Layout.fillWidth: true | ||
| Layout.preferredHeight: 40 | ||
| visible: root.searchModel && !root.aggregateView | ||
|
|
||
| ToolButton { | ||
| id: backButton | ||
|
|
||
| objectName: "searchDetailBackButton" | ||
| anchors.left: parent.left | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| text: qsTr("Back") | ||
| icon.source: "image://svgimage-custom-color/" | ||
| + (root.LayoutMirroring.enabled ? "arrow-right.svg/" : "arrow-left.svg/") | ||
| + Style.wizardPrimaryText | ||
| icon.width: Style.smallIconSize | ||
| icon.height: Style.smallIconSize | ||
| display: AbstractButton.TextBesideIcon | ||
| Accessible.name: qsTr("Back to all search results") | ||
| onClicked: { | ||
| root.searchModel.closeProviderDetail() | ||
| root.focusSearchInput() | ||
| } | ||
| } | ||
| EnforcedPlainTextLabel { | ||
| objectName: "searchDetailProviderTitle" | ||
| anchors.left: parent.left | ||
| anchors.right: parent.right | ||
| anchors.leftMargin: backButton.width + Style.smallSpacing | ||
| anchors.rightMargin: backButton.width + Style.smallSpacing | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| text: root.searchModel ? root.searchModel.detailProviderName : "" | ||
| font.bold: true | ||
| font.pixelSize: Style.wizardHeaderTitleFontPixelSize | ||
| elide: Text.ElideRight | ||
| horizontalAlignment: Text.AlignHCenter | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this is large enough that it should live in its own file
| Flow { | ||
| id: filterFlow | ||
| objectName: "categoryFilterFlow" | ||
| Layout.fillWidth: true | ||
| Layout.preferredHeight: visible ? childrenRect.height : 0 | ||
| visible: root.filtersVisible | ||
| spacing: Style.smallSpacing | ||
|
|
||
| WizardButton { | ||
| id: typeFilterButton | ||
|
|
||
| objectName: "typeFilterButton" | ||
| width: Math.max(140, (filterFlow.width - 2 * filterFlow.spacing) / 3) | ||
| text: qsTr("Type") | ||
| trailingIconSource: "image://svgimage-custom-color/caret-down.svg/" | ||
| + (primary ? Style.wizardSelectedText : Style.wizardPrimaryText) | ||
| iconBeforeText: true | ||
| iconSource: "image://svgimage-custom-color/folder.svg/" | ||
| + (primary ? Style.wizardSelectedText : Style.wizardPrimaryText) | ||
| primary: root.hasActiveFilter("provider") | ||
| Accessible.name: qsTr("Filter by type") | ||
| onClicked: typeMenu.toggle() | ||
| WizardMenu { | ||
| id: typeMenu | ||
| objectName: "typeFilterMenu" | ||
| anchorItem: typeFilterButton | ||
| width: anchorItem.width * 1.5 | ||
| Repeater { | ||
| model: root.searchModel ? root.searchModel.providers : [] | ||
| delegate: WizardMenuItem { | ||
| required property var modelData | ||
| text: (modelData.selected ? "✓ " : "") + modelData.name | ||
| icon.source: modelData.icon ? "image://tray-image-provider/" + modelData.icon : "" | ||
| tintIcon: true | ||
| iconTintColor: Style.wizardPrimaryText | ||
| onTriggered: root.searchModel.toggleProviderFilter(modelData.id) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| onClearText: { | ||
| if (root.searchModel) { | ||
| root.searchModel.searchTerm = "" | ||
| WizardButton { | ||
| id: dateFilterButton | ||
|
|
||
| objectName: "dateFilterButton" | ||
| width: Math.max(140, (filterFlow.width - 2 * filterFlow.spacing) / 3) | ||
| text: qsTr("Date") | ||
| trailingIconSource: "image://svgimage-custom-color/caret-down.svg/" | ||
| + (primary ? Style.wizardSelectedText : Style.wizardPrimaryText) | ||
| iconBeforeText: true | ||
| iconSource: "image://svgimage-custom-color/calendar.svg/" | ||
| + (primary ? Style.wizardSelectedText : Style.wizardPrimaryText) | ||
| primary: root.hasActiveFilter("date") | ||
| enabled: root.searchModel && root.searchModel.dateFilterAvailable | ||
| Accessible.name: qsTr("Filter by date") | ||
| Accessible.description: enabled ? "" : qsTr("No search source supports date filtering") | ||
| onClicked: dateMenu.toggle() | ||
| WizardMenu { | ||
| id: dateMenu | ||
| objectName: "dateFilterMenu" | ||
| anchorItem: dateFilterButton | ||
| WizardMenuItem { | ||
| objectName: "dateTodayMenuItem" | ||
| text: qsTr("Today") | ||
| onTriggered: root.searchModel.setDatePreset("today") | ||
| } | ||
| WizardMenuItem { text: qsTr("Last 7 days"); onTriggered: root.searchModel.setDatePreset("last7days") } | ||
| WizardMenuItem { text: qsTr("Last 30 days"); onTriggered: root.searchModel.setDatePreset("last30days") } | ||
| WizardMenuItem { text: qsTr("This year"); onTriggered: root.searchModel.setDatePreset("thisyear") } | ||
| WizardMenuItem { text: qsTr("Last year"); onTriggered: root.searchModel.setDatePreset("lastyear") } | ||
| MenuSeparator {} | ||
| WizardMenuItem { | ||
| text: qsTr("Custom range …") | ||
| onTriggered: { | ||
| customRangeDialog.validationError = false | ||
| customRangeDialog.open() | ||
| } | ||
| } | ||
| WizardMenuItem { text: qsTr("Clear date"); onTriggered: root.searchModel.clearDateFilter() } | ||
| } | ||
| } | ||
| WizardButton { | ||
| id: peopleButton | ||
| objectName: "peopleFilterButton" | ||
| width: Math.max(140, (filterFlow.width - 2 * filterFlow.spacing) / 3) | ||
| text: qsTr("People") | ||
| trailingIconSource: "image://svgimage-custom-color/caret-down.svg/" | ||
| + (primary ? Style.wizardSelectedText : Style.wizardPrimaryText) | ||
| iconBeforeText: true | ||
| iconSource: "image://svgimage-custom-color/account-group.svg/" | ||
| + (primary ? Style.wizardSelectedText : Style.wizardPrimaryText) | ||
| primary: root.hasActiveFilter("person") | ||
| enabled: root.searchModel && root.searchModel.peopleFilterAvailable | ||
| Accessible.name: qsTr("Filter by person") | ||
| Accessible.description: enabled ? "" : qsTr("No search source supports people filtering") | ||
| onClicked: peoplePopup.open() | ||
| } | ||
| } |
| delegate: Item { | ||
| id: delegateData | ||
|
|
||
| required property string providerName | ||
| required property string providerId | ||
| required property string providerIcon | ||
| required property string resultTitle | ||
| required property string subline | ||
| required property url resourceUrlRole | ||
| required property string darkIcons | ||
| required property string lightIcons | ||
| required property bool darkIconsIsThumbnail | ||
| required property bool lightIconsIsThumbnail | ||
| required property string darkImagePlaceholder | ||
| required property string lightImagePlaceholder | ||
| required property bool isRounded | ||
| required property int type | ||
| required property bool isSelected | ||
| required property bool isPartialMatch | ||
| required property bool hasOverflow | ||
| required property bool isLoading | ||
|
|
||
| readonly property alias loadedItem: resultDelegate.loadedItem | ||
|
|
||
| width: resultsList.width | ||
| height: resultDelegate.implicitHeight | ||
|
|
||
| UnifiedSearchResultDelegate { | ||
| id: resultDelegate | ||
|
|
||
| anchors.fill: parent | ||
| searchModel: root.searchModel | ||
| providerName: delegateData.providerName | ||
| providerId: delegateData.providerId | ||
| providerIcon: delegateData.providerIcon | ||
| resultTitle: delegateData.resultTitle | ||
| subline: delegateData.subline | ||
| resourceUrlRole: delegateData.resourceUrlRole | ||
| darkIcons: delegateData.darkIcons | ||
| lightIcons: delegateData.lightIcons | ||
| darkIconsIsThumbnail: delegateData.darkIconsIsThumbnail | ||
| lightIconsIsThumbnail: delegateData.lightIconsIsThumbnail | ||
| darkImagePlaceholder: delegateData.darkImagePlaceholder | ||
| lightImagePlaceholder: delegateData.lightImagePlaceholder | ||
| isRounded: delegateData.isRounded | ||
| resultType: delegateData.type | ||
| isSelected: delegateData.isSelected | ||
| isPartialMatch: delegateData.isPartialMatch | ||
| hasOverflow: delegateData.hasOverflow | ||
| isLoading: delegateData.isLoading | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are we having a delegate within a delegate?
| Popup { | ||
| id: peoplePopup | ||
| parent: Overlay.overlay | ||
| width: Math.min(root.width - 40, 420) | ||
| height: 340 | ||
| x: (root.width - width) / 2 | ||
| y: 150 | ||
| modal: true | ||
| closePolicy: Popup.CloseOnEscape | Popup.CloseOnPressOutside | ||
| onOpened: peopleSearch.forceActiveFocus() | ||
| ColumnLayout { | ||
| anchors.fill: parent | ||
| anchors.margins: Style.smallSpacing | ||
| TextField { | ||
| id: peopleSearch | ||
| Layout.fillWidth: true | ||
| placeholderText: qsTr("Search people") | ||
| onTextEdited: peopleModel.searchTerm = text | ||
| } | ||
| EnforcedPlainTextLabel { visible: peopleModel.errorString.length > 0; text: peopleModel.errorString; wrapMode: Text.Wrap } | ||
| WizardButton { | ||
| visible: peopleModel.errorString.length > 0 | ||
| text: qsTr("Retry") | ||
| onClicked: peopleModel.retry() | ||
| } | ||
| ListView { | ||
| Layout.fillWidth: true | ||
| Layout.fillHeight: true | ||
| clip: true | ||
| model: peopleModel | ||
| delegate: ItemDelegate { | ||
| id: personDelegate | ||
| required property string userId | ||
| required property string displayName | ||
| required property string avatarUrl | ||
| width: ListView.view.width | ||
| height: 44 | ||
| text: displayName | ||
| hoverEnabled: true | ||
| Accessible.description: userId | ||
| background: Rectangle { | ||
| color: personDelegate.hovered || personDelegate.down | ||
| ? Style.listItemHoverBackground | ||
| : "transparent" | ||
| radius: Style.mediumRoundedButtonRadius | ||
| } | ||
| HoverHandler { | ||
| cursorShape: Qt.PointingHandCursor | ||
| } | ||
| contentItem: RowLayout { | ||
| Image { | ||
| Layout.preferredWidth: 32 | ||
| Layout.preferredHeight: 32 | ||
| sourceSize.width: 32 | ||
| sourceSize.height: 32 | ||
| asynchronous: true | ||
| source: personDelegate.avatarUrl.length > 0 | ||
| ? "image://tray-image-provider/" + personDelegate.avatarUrl : "" | ||
| Accessible.ignored: true | ||
| } | ||
| EnforcedPlainTextLabel { Layout.fillWidth: true; text: personDelegate.displayName; elide: Text.ElideRight } | ||
| } | ||
| onClicked: { | ||
| root.searchModel.setPersonFilter(userId, displayName, avatarUrl) | ||
| peoplePopup.close() | ||
| root.focusSearchInput() | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Dialog { | ||
| id: customRangeDialog | ||
| property bool validationError: false | ||
| anchors.centerIn: parent | ||
| title: qsTr("Custom date range") | ||
| modal: true | ||
|
|
||
| footer: RowLayout { | ||
| spacing: Style.wizardFooterSpacing | ||
|
|
||
| Item { | ||
| Layout.fillWidth: true | ||
| } | ||
|
|
||
| WizardButton { | ||
| text: qsTr("Cancel") | ||
| onClicked: customRangeDialog.close() | ||
| } | ||
|
|
||
| WizardButton { | ||
| primary: true | ||
| text: qsTr("Apply") | ||
| onClicked: { | ||
| customRangeDialog.validationError = !root.searchModel.setCustomDateRange(customSince.text, customUntil.text) | ||
| if (!customRangeDialog.validationError) { | ||
| customRangeDialog.close() | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ColumnLayout { | ||
| EnforcedPlainTextLabel { text: qsTr("Start date (YYYY-MM-DD)") } | ||
| TextField { id: customSince; Layout.fillWidth: true; placeholderText: "YYYY-MM-DD" } | ||
| EnforcedPlainTextLabel { text: qsTr("End date (YYYY-MM-DD)") } | ||
| TextField { id: customUntil; Layout.fillWidth: true; placeholderText: "YYYY-MM-DD" } | ||
| EnforcedPlainTextLabel { | ||
| visible: customRangeDialog.validationError | ||
| text: qsTr("Enter valid dates with the start date before the end date.") | ||
| color: palette.accent | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These dialogs and popups should be moved into their own files too
| switch (role) { | ||
| case UserIdRole: return person.id; | ||
| case DisplayNameRole: return person.displayName; | ||
| case AvatarUrlRole: return person.avatarUrl; | ||
| } | ||
| return {}; |
There was a problem hiding this comment.
Is clang-format happy with this?
| implicitWidth: contentRow.implicitWidth | ||
| implicitHeight: contentRow.implicitHeight | ||
| contentItem: RowLayout { | ||
| spacing: 6 |
There was a problem hiding this comment.
Please no magic numbers
| color: root.enabled | ||
| ? (root.primary ? Style.wizardSelectedText : root.palette.buttonText) | ||
| : Style.wizardDisabledText |
There was a problem hiding this comment.
Ternary chains are readability nightmares, let's just use a conventional if/else
| leftPadding: Style.standardSpacing | ||
| rightPadding: Style.standardSpacing |
There was a problem hiding this comment.
Do we really want to hardcode padding into a button type?
| leftPadding: 12 | ||
| rightPadding: 12 |
There was a problem hiding this comment.
No magic numbers please
| } | ||
|
|
||
| auto *const searchModel = new UnifiedSearchResultsListModel(accountState.data(), accountState.data()); | ||
| auto *const searchModel = new UnifiedSearchResultsListModel(accountState.data()); |
There was a problem hiding this comment.
| auto *const searchModel = new UnifiedSearchResultsListModel(accountState.data()); | |
| const auto searchModel = new UnifiedSearchResultsListModel(accountState.data()); |
e5e8dda to
8fbc4d6
Compare
Modernize the account-scoped search window with provider, date, and people filters, connected-service results, and aggregate and provider detail views. Introduce per-provider search state, pagination, scoped retries, keyboard selection, shared wizard styling, and model and QML coverage. Co-authored-by: Matthieu Gallien <matthieu.gallien@nextcloud.com> Signed-off-by: Rello <github@scherello.de> Assisted-by: Codex:GPT-5
8fbc4d6 to
717974c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 717974c845
ℹ️ 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".
| { | ||
| const auto encodedUserId = QString::fromUtf8(QUrl::toPercentEncoding(userId)); | ||
| const auto avatarPath = QStringLiteral("index.php/avatar/%1/64").arg(encodedUserId); | ||
| return account->url().resolved(QUrl(avatarPath)).toString(); |
There was a problem hiding this comment.
For accounts hosted below the domain root whose URL lacks a trailing slash, such as https://host/nextcloud, QUrl::resolved() treats nextcloud as a file and produces https://host/index.php/avatar/.... Consequently, every person avatar fails to load on these deployments; use the existing Utility::concatUrlPath() helper and add a subdirectory-account regression case.
AGENTS.md reference: AGENTS.md:L33-L35
Useful? React with 👍 / 👎.
| QVector<UnifiedSearchResult> projection; | ||
| if (_viewMode == ViewMode::ProviderDetail && _providers.contains(_detailProviderId)) { | ||
| const auto &provider = _providers[_detailProviderId]; | ||
| projection = provider.entries; |
There was a problem hiding this comment.
When a date or person filter is active and a provider does not support it, the aggregate projection marks that provider's rows as partial, but entering its detail view copies the raw entries whose _isPartialMatch remains false. Because the filter controls and partial-match header are also hidden in detail view, the unfiltered results are presented as ordinary matches; carry the provider's partial state into this projection or retain an equivalent warning.
Useful? React with 👍 / 👎.
| onPressed: { | ||
| if (root.hasOverflow) { | ||
| root.searchModel.openProviderDetail(root.providerId) |
There was a problem hiding this comment.
On touch input, a drag beginning on an overflow provider header emits pressed before the ListView can take over the gesture, so attempting to scroll immediately opens the provider detail even if the press is subsequently canceled. The paging row has the same issue and can start a network request during scrolling; handle these actions with onClicked so canceled presses do not activate them.
Useful? React with 👍 / 👎.
|
Artifact containing the AppImage: nextcloud-appimage-pr-10639.zip Digest: 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. |
Server Reference: nextcloud/server#60241
Assisted-by: Codex:GPT-5
Bildschirmaufnahme.2026-08-21.um.11.22.02.mov