what does it means? is it done already?

@msciubidlo See https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

About the UI, I noticed that when the panel is in "configure" mode, it's still possible to right-click it and click the "Configure" entry, which will do nothing.
We could fix this by disabling the entry once clicked. Another idea: make the entry checkable instead of using the buttonbox.

Please bump the minimum required version for KF5BalooWidgets to 19.07.70.

Another thing: can you have a look about these warnings?

Thanks

This made me think: what about non-plasma systems?
e.g. GNOME doesn't even have a taskbar, what's gonna happen there?

Hmm, it seems you need to rebase this patch:

In D20452#448644, @alexde wrote:

This patch looks more like a workaround to me. I'd suggest to identify the root cause of "^C" and "cd <path>" whenever the terminal is hidden and reopened again by hitting F4 and to find a another solution as I guess and especially hope that this behaviour is actually not necessary to happen.

Hmm, I don't I like it sorry. Running clear without the user permission could be annoying or even dangerous.

In D16648#447625, @ngraham wrote:

@elvisangelaccio is this the patch we're going with?

Sorry for the delay. Let's focus on D16648 please, which has already started its review process.

Please push to master only, just to make sure we don't introduce regressions.

In D20152#446323, @trmdi wrote:

In D20152#446262, @elvisangelaccio wrote:

In D20152#445778, @trmdi wrote:

In D20152#445537, @elvisangelaccio wrote:

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

I don't think it is needed because it was set in the above code block.

Only if m_selectionManager->currentItem() != index, but this won't be true when the item is not visible. Which is why you want to move the scrollToItem() call outside this if() block in the first place, isn't it?

When m_selectionManager->currentItem() == index, the setCurrentItem(index) will do nothing.

In D20152#445778, @trmdi wrote:

In D20152#445537, @elvisangelaccio wrote:

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

I don't think it is needed because it was set in the above code block.

I didn't notice that this patch is also adding click-on-preview to toggle play/pause. To me that is an unrelated feature that should go in another commit (and will probably help you to debug it).

Thanks. Please push to Applications/19.04

We always call KItemListSelectionManager::setCurrentItem() before calling scrollToItem(), so we should probably do it also here.

Let's see what the dolphin philosophy says: https://github.com/KDE/dolphin/blob/master/HACKING.md

About the UI: I'd also be in favor of not showing the arrows. It's not just Falkon that doesn't show them, but also Firefox and Chrome.

Too late for 19.04 anyway, sorry for the delay :(

Too late for 19.04, please push to master (after replacing BUG: with FEATURE: in the commit message :D)

Is it really necessary to copy urlinfo.h here? Wouldn't be enough to just create a static function and put the URL parsing logic there?

I cannot open the test file...

In D20024#440855, @hallas wrote:

Could you try and move the testHideItem to be the first test? Also make sure to run placesitemmodeltest using valgrind, i.e. valgrind placesitemmodeltest

In D20024#440353, @hallas wrote:

Hi @elvisangelaccio , I have been looking into writing a unit test for the crash, but it turns out we already have one: PlacesModelItemTest::testHideItem. But, the test only triggers the crash if it is run before the testPlaceItem test case. I have been digging in the code to figure out why the test cases are coupled in this way, but I haven't figured it out, so could you take a look? Here is a diff that triggers the crash:

diff --git a/src/tests/placesitemmodeltest.cpp b/src/tests/placesitemmodeltest.cpp
index fac0931a6..05aa02064 100644
--- a/src/tests/placesitemmodeltest.cpp
+++ b/src/tests/placesitemmodeltest.cpp
@@ -63,13 +63,13 @@ private slots:
     void testModelSort();
     void testGroups();
     void testDeletePlace();
+    void testHideItem();
     void testPlaceItem_data();
     void testPlaceItem();
     void testTearDownDevice();
     void testDefaultViewProperties_data();
     void testDefaultViewProperties();
     void testClear();
-    void testHideItem();
     void testSystemItems();
     void testEditBookmark();
     void testEditAfterCreation();

@rthomsen Is it possible to add a small test archive so that we can write an unit test?

Bonus point if you add a test case in urltest.cpp ;)

Please update the commit message before pushing, it's still referring to PercentFromNestedJob

Don't forget to update the commit message (it still mentions "Add GetProcessList") ;)

I get a crash if I run dolphin /etc/fstab:

Please push to master only.

That said, the fix looks correct. An unit test for this crash would be awesome, you have spare time.

Hmm, I can't reproduce the crash following the test plan.

Yes please, this feature really belongs to KUrlNavigator.

• David's suggestion

Generally UI changes shouldn't go to the stable branch, but this can be considered a bugfix, so let's go with 19.04 ;)

In D19926#437288, @ngraham wrote:

Hmm, when I apply this patch I don't actually see the new menu item.

If it is possible to somehow force Qt to connect to the signal, then the PercentFromNestedJob is not necessary.

What about the Control menu? Remember that the menubar is hidden by default in dolphin, so the average user probably won't be able to discover this feature.

Also please write a descriptive commit message and use the BUG: keyword. See https://community.kde.org/Policies/Commit_Policy#Special_keywords_in_GIT_and_SVN_log_messages

In D19987#436669, @krutovmikhail wrote:

Oh. Alright. How do I close this MR?

Makes sense, thanks for the explanation. Reverted with 3fe6547197.

In D19831#437049, @davidedmundson wrote:

We don't want more stuff using khotkeys, you've seen the spectacle discussion and the move to the new kglobalaccel code.

Please push to the 19.04 branch. Thanks!

I see. Then I'd exclude #3 for the reason you mentioned. I'd be ok with either #1 or #2, with a slight preference for #2.

Thanks, but there is already a patch for this in D19950. We may want to focus on that one.

In D19950#436013, @tigrang wrote:

I undid changes to Trash. The only difference now is Mount/Unmount is lower in the list.

I'd also try to avoid copying the code here, is possible.

@sitter @bcooksley It seems the freebsd CI node doesn't have the test-unit gem, can we install it?

We are still in time for 19.04 ;)

Haven't tried D19844 yet, but this patch can only make sense.

What if the user press Cancel instead of closing the window?

In D19661#434051, @ngraham wrote:

BTW, I'm not sure how it could possibly be related, but the placesitemmodel test has started consistently failing after this commit went in: https://build.kde.org/job/Applications/job/dolphin/job/stable-kf5-qt5%20SUSEQt5.10/30/testReport/junit/projectroot.src/tests/placesitemmodeltest/