When you search for a file (e.g. "hello world") the window title of Dolphin will now change to "Search for [input]" instead of "baloosearch - /".
BUG: 321575
Details
Details
- Reviewers
ngraham elvisangelaccio - Group Reviewers
Dolphin - Commits
- R318:702429ac8e64: Change window title when searching
- Disables it iself after search bar is hidden
- Works with multiple word searches
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
I'm excited about this, having been banging my head against the problem for a week from a different angle, without success.
That said, I'm afraid the patch doesn't apply cleanly against current master:
$ arc patch D8273
Created and checked out branch arcpatch-D8273.
Checking patch src/dolphinviewcontainer.cpp...
Hunk #1 succeeded at 245 (offset 11 lines).
Checking patch src/dolphinviewcontainer.h...
Checking patch src/dolphinmainwindow.cpp...
Hunk #1 succeeded at 33 (offset 1 line).
error: while searching for:
caption.append(fileName);
}
setWindowTitle(caption);
}
error: patch failed: src/dolphinmainwindow.cpp:976
Applied patch src/dolphinviewcontainer.cpp cleanly.
Applied patch src/dolphinviewcontainer.h cleanly.
Applying patch src/dolphinmainwindow.cpp with 1 reject...
Hunk #1 applied cleanly.
Rejected hunk #2.
Patch Failed!
Usage Exception: Unable to apply patch!
Can you re-base it and try again?
Also, we can only accept patches from people with public email addresses and real names. Any chance we can persuade you to be known by something other than "XY Quadrat"?
Comment Actions
Split the "caption" variable into two variables (schemePrefix & fileName) to reflect changes made from master since I sent this to review for the first time.
(Also, I have changed my real name @ KDE Identity, but Phabricator somehow does not want to apply this change. Any ideas?)
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1003 | This will lead to some tricky corner cases producing wrong window titles (like baloosearch - /), when the URL is a search URL but the state of the search box doesn't reflect it at this point (depends on the ordering of the method calls). Better use the information available in the url, e.g.:
if (url.scheme() == "baloosearch") {
#ifdef HAVE_BALOO
return Baloo::Query::titleFromQueryUrl(url);
#endif
} else if (url.scheme() == "filenamesearch") {
return QString("Query Results from '%1'").arg(url.queryItemValue("search"));
}
//... existing code .... | |
Comment Actions
Great work. A few suggestions:
- Instead of "Searching [search term]", how about "Search for [search term]"?
- For Dolphin's built in "Search for" items (Documents, Images, Audio Files, Videos) in the Places panel, how about "Search for [type]" when there's no search term, and then when the user adds a search term, it changes to "Search for [thing] named [search term]".
Comment Actions
@emmanuelp Am I correct if I assume that I can then delete the changes made in dolphinviewcontainer.cpp/.h as the currentSearchText() function would then no longer be needed?
Comment Actions
/** * Sets a context that is used for remembering the view-properties. * Per default the context is empty and the path of the currently set URL * is used for remembering the view-properties. Setting a custom context * makes sense if specific types of URLs (e.g. search-URLs) should * share common view-properties. */ void setViewPropertiesContext(const QString& context); QString viewPropertiesContext() const;
I think you want that, it's in every view object (DolphinView).
You can probably get the property with: m_activeViewContainer->view()->viewPropertiesContext()
Right now, the viewPropertiesContext is search when in a search, but i don't know if that's after you typed in a query and pressed enter or as soon as the search box is open.
If it's the latter, then you should probably expand the viewPropertiesContext to say (for instance) "search" when the box is open, but enter isn't pressed yet and "search-results" when results are shown.
Note, you also have the "isSearchUrl(const QUrl &url)" method in DolphinViewContainer. It probably does exactly what you want, but imho it's check to determine the "isSearchUrl" is rather weak. It simply does a string contains on the scheme which evaluates to true if "search" is in the scheme. It probably works just fine.
Just my 5 cents :)
Comment Actions
@xyquadrat, have you had a chance to look into any of the comments folks have posted?
Comment Actions
I have looked at all of them, and now I am actually a bit overstrained with all the possibilities. I am not sure which function to use to check if the user is searching as my earlier solutions seems to fail in some edge cases. Additionally, I tried to implement your request "Search for [type]" but in the newest master branch, all search options are greyed out -> I can't test my solution.
Sorry that I didn't respond earlier, life has been busy.
Comment Actions
It's all good, no rush. :) Just trying to make sure this doesn't get lost, since it's a good fix (even as is, without any of my suggestions).
Comment Actions
- Changed "Searching [user input]" to "Search for [user input]"
- Instead of using isSearchModeEnabled() I now use viewPropertyContext(), although I am not 100% sure if that will work with all corner cases (@emmanuelp probably knows that better)
I also tried to implement the suggestion to mention the type of search (e.g. for Documents). My problem is that the method to get the current facet (facetType()) is buried in dolphinfacetswidget.cpp, which I can't seem to access from dolphinmainwindow.cpp. Any ideas?
Comment Actions
You might consider adding the code to change the title into src/search/dolphinsearchbox.cpp; then you could easily access the facets.
Comment Actions
This revision adds the feature proposed by @ngraham. The search text will now change to "Search for [type] named [input]" or "Search for [type]" if the user did not input a search term.
But. I created two additional helper functions (currentFacet() which calls getFacet()) because the facet type is private from dolphinmainwindow.cpp. Ngraham proposed to move everything into dolphinsearchbox.cpp, but then you can no longer sync the window title update with the ones from dolphinmainwindows.cpp.
I know that this inflates the patch size a lot, and there is probably a better way to do it, but I just can't find it. So feedback is really appreciated :)
Comment Actions
Hmm, the patch doesn't apply for me using arc patch:
error: patch failed: src/dolphinmainwindow.cpp:1003
Applied patch src/search/dolphinsearchbox.h cleanly.
Applied patch src/search/dolphinsearchbox.cpp cleanly.
Applying patch src/dolphinviewcontainer.h with 1 reject...
Rejected hunk #1.
Applying patch src/dolphinviewcontainer.cpp with 1 reject...
Rejected hunk #1.
Applying patch src/dolphinmainwindow.cpp with 1 reject...
Rejected hunk #1.
If you haven't gotten set up with arc yet, I highly recommend it. It's actually a breeze to get up and running, and it makes this process sooooo much smoother.
Comment Actions
Weird, when I do
arc patch D8273
on the lastest master, the patch applies perfectly fine for me.
Comment Actions
It seems you have some local commits not pushed, try to do a clone from scratch and it should fail also for you.
Comment Actions
I am not very experienced with git and arc , but I think that everything should work now. Sorry for all the trouble with this patch.
Comment Actions
The good news: the patch now applies cleanly with arc patch!
The bad news: the change itself seems to be broken now. window titles still say "baloosearch - /" everywhere.
Comment Actions
Hmm, I tested it multiple times (also with a completely fresh copy & applying the patch) and it always worked for me.
Comment Actions
Patch works for me as well now.
@xyquadrat What's the difference with the old patch in https://git.reviewboard.kde.org/r/130157/ ?
Comment Actions
Function should looks like:
void DolphinMainWindow::setUrlAsCaption(const QUrl& url)
{
static KFilePlacesModel s_placesModel;
if (url.scheme().contains(QStringLiteral("search"))) {
const auto search = i18n("Searching for ");
const auto text = m_activeViewContainer->currentSearchText();
setWindowTitle(search + text);
return;
}
// ... rest of the function
}| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1004 | if[space]( and )[space]{in every place. | |
| 1006 | Always use isEmpty do not compare with empty null-terminated string. | |
| 1009 | You cannot construct translated text in this way. Translated strings should be whole sentence. | |
| src/search/dolphinsearchbox.cpp | ||
| 572 ↗ | (On Diff #22221) | getXXX is not used as template name, use facetType instead. |
Comment Actions
@elvisangelaccio The basic patch is the same, but the "Places" feature has been added & some code style changes were made.
@anthonyfieroni I tried to adapt the code to your suggestions, any improvements from here?
When the user selected a facet but hasn't entered any text the title is now "Search for [type] named". I can re-add the change so that it would be "Search for [type]" but this would inflate the patch size and IMO is about as good as it is now.
Comment Actions
It looks good to me +1
// Edit
I made conditional statement at top of the function because if full path is setted in settings baloosearch or filenamesearch will present again. Notice line 986
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1004 | Old patch was using if (m_activeViewContainer->isSearchModeEnabled()), why did you change it? (should be faster as it doesn't perform a substring match). | |
| 1007 | I'm not sure it's worth putting the facets in the title. Currently I see the following issues:
| |
| 1009 | Same here, use %1 as placeholder for text. | |
Comment Actions
Removed the facetType() and currentFacet() function as proposed by @elvisangelaccio. Merged the search and text variable into searchText, which now uses %1 to add in the current search text for better localization. But I am not entirely sure if the consent is that we remove the facets from the title (which gives more space) or if I should keep it (which gives more information to the user) but adapt to the other comments.
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1004 | I thought your earlier comment implied that isSearchModeEnabled() will not work in some corner cases, but apparently I misunderstood that. | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1005 | Please add a check if the search query is empty. Currently it will show "Search for " which doesn't look good. | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1005 | I can easily add a check for that, but what should it show instead? "Starting search"? | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1006 | Please use title capitalization ("Empty Search") Btw the local variable is not needed, we could just do setWindowTitle(i18n(...)) | |
| src/dolphinmainwindow.cpp | ||
|---|---|---|
| 1006 | What I meant is "Empty Search" rather than "Empty search". See https://community.kde.org/KDE_Visual_Design_Group/HIG/Capitalization | |