Differential D8273 Diff 23349 src/dolphinmainwindow.cpp

Changeset View

Standalone View

View Options

src/dolphinmainwindow.cpp

Show First 20 Lines • Show All 994 Lines • ▼ Show 20 Line(s)994 if (!matchedPlaces.isEmpty()) {
995 setWindowTitle(s_placesModel.text(matchedPlaces.first())); 995​ setWindowTitle(s_placesModel.text(matchedPlaces.first()));
996 return; 996​ return;
997 } 997​ }
998 998​
999 QString fileName = url.adjusted(QUrl::StripTrailingSlash).fileName(); 999​ QString fileName = url.adjusted(QUrl::StripTrailingSlash).fileName();
1000 if (fileName.isEmpty()) { 1000​ if (fileName.isEmpty()) {
1001 fileName = '/'; 1001​ fileName = '/';
1002 } 1002​ }
1003 1003​
emmanuelpUnsubmitted
Done

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 is baloosearch use Baloo::Query::titleFromQueryUrl(url)
  • If URL scheme is 'filenamesearch` use QString("Query Results from '%1'").arg(url.queryItemValue("search"))
    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 ....
emmanuelp: This will lead to some tricky corner cases producing wrong window titles (like `baloosearch…
1004​ if (m_activeViewContainer->isSearchModeEnabled()) {
anthonyfieroniUnsubmitted
Done
if[space]( and )[space]{

in every place.

anthonyfieroni: ``` if[space]( and )[space]{ ``` in every place.
elvisangelaccioUnsubmitted
Done

Old patch was using if (m_activeViewContainer->isSearchModeEnabled()), why did you change it? (should be faster as it doesn't perform a substring match).

elvisangelaccio: Old patch was using `if (m_activeViewContainer->isSearchModeEnabled())`, why did you change it?
xyquadratAuthorUnsubmitted
Not Done

I thought your earlier comment implied that isSearchModeEnabled() will not work in some corner cases, but apparently I misunderstood that.

xyquadrat: I thought your earlier comment implied that `isSearchModeEnabled()` will not work in some…
1005​ if(m_activeViewContainer->currentSearchText().isEmpty()){
elvisangelaccioUnsubmitted
Done

Please add a check if the search query is empty. Currently it will show "Search for " which doesn't look good.

elvisangelaccio: Please add a check if the search query is empty. Currently it will show "Search for " which…
xyquadratAuthorUnsubmitted
Not Done

I can easily add a check for that, but what should it show instead? "Starting search"?

xyquadrat: I can easily add a check for that, but what should it show instead? "Starting search"?
1006​ setWindowTitle(i18n("Empty Search"));
anthonyfieroniUnsubmitted
Done

Always use isEmpty do not compare with empty null-terminated string.

anthonyfieroni: Always use isEmpty do not compare with empty null-terminated string.
elvisangelaccioUnsubmitted
Done

Please use title capitalization ("Empty Search")

Btw the local variable is not needed, we could just do setWindowTitle(i18n(...))

elvisangelaccio: Please use title capitalization ("Empty Search") Btw the local variable is not needed, we…
elvisangelaccioUnsubmitted
Done

What I meant is "Empty Search" rather than "Empty search". See https://community.kde.org/KDE_Visual_Design_Group/HIG/Capitalization

elvisangelaccio: What I meant is "Empty Search" rather than "Empty search". See https://community.kde.
1007​ } else {
elvisangelaccioUnsubmitted
Done

I'm not sure it's worth putting the facets in the title. Currently I see the following issues:

  1. We should not concatenate i18n strings, there should be a %2 placeholder for text.
  2. If I select the Folder facet but I don't type anything, I get Search for Folder named .
  3. Facets are not translated.
elvisangelaccio: I'm not sure it's worth putting the facets in the title. Currently I see the following issues…
1008​ const auto searchText = i18n("Search for %1", m_activeViewContainer->currentSearchText());
1009​ setWindowTitle(searchText);
anthonyfieroniUnsubmitted
Done

You cannot construct translated text in this way. Translated strings should be whole sentence.

anthonyfieroni: You cannot construct translated text in this way. Translated strings should be whole sentence.
elvisangelaccioUnsubmitted
Done

Same here, use %1 as placeholder for text.

elvisangelaccio: Same here, use `%1` as placeholder for `text`.
1010​ }
1011​ return;
1012​ }
1013​
1004 setWindowTitle(schemePrefix + fileName); 1014​ setWindowTitle(schemePrefix + fileName);
1005} 1015​}
1006 1016​
1007void DolphinMainWindow::slotStorageTearDownFromPlacesRequested(const QString& mountPath) 1017​void DolphinMainWindow::slotStorageTearDownFromPlacesRequested(const QString& mountPath)
1008{ 1018​{
1009#ifndef Q_OS_WIN 1019​#ifndef Q_OS_WIN
1010 if (m_terminalPanel->currentWorkingDirectory().startsWith(mountPath)) { 1020​ if (m_terminalPanel->currentWorkingDirectory().startsWith(mountPath)) {
1011 m_tearDownFromPlacesRequested = true; 1021​ m_tearDownFromPlacesRequested = true;
▲ Show 20 Lines • Show All 580 Lines • Show Last 20 Lines