SUMMARY In the image slideshow, when a random order is selected and all items of the current list were consumed and the next set of items is queued, the solution for avoiding a duplicate item selection in the transition is not ideal and leads to skipping one of the items. The `ImageBackend::nextSlide()` function currently does this: https://invent.kde.org/plasma/plasma-workspace/-/blob/v6.4.90/wallpapers/image/plugin/imagebackend.cpp#L411-420 - When the end of the slideshow has been reached (index is 0 again), reshuffle it (`invalidate()`) - Set the next item to the item with index 0 - If the previous item is the same as the next item, increase the index by 1 and set next that instead This means that the item with index 0 will not be shown in this case, as it's simply skipped due to it being the same as the previous one, as a valid wallpaper transition needs to be ensured. A more sensible idea would be swapping the first item with item [2...n] of the reshuffled list, so that no item is skipped and every item will be shown. From looking at the code (I'm neither familiar with C++/Qt nor with the plugin's implementation), it seems like this was the simplest solution, as there's no interface on the `SlideFilterModel` class for swapping specific items: https://invent.kde.org/plasma/plasma-workspace/-/blob/v6.4.90/wallpapers/image/plugin/slidefiltermodel.h#L17 Alternatively, the slideshow could be reshuffled again until the first item is not the previous one, but this of course doesn't work in constant time. STEPS TO REPRODUCE Have a wallpaper image slideshow in random order with at least 3 items OBSERVED RESULT Current slideshow: A B C Potential next slideshows: A B C A C B B A C B C A A B (C was skipped) B A (C was skipped) EXPECTED RESULT Current slideshow: A B C Potential next slideshows: A B C A C B B A C B C A A C B (via C A B) B A C (via C A B) B C A (via C B A) A B C (via C B A)
Is it a real problem? C will shown in the next round
(In reply to David Redondo from comment #1) > Is it a real problem? C will shown in the next round No, it's a very minor issue, but I think that this still deserves a fix, if it's trivial to fix. Skipping an item in the queue means that the queue size is reduced by one, so if a user expects their wallpaper slideshow to repeat in strict cycles, even though the order of items is randomized, then this cycle shifts over time, which can be annoying. (Yes, I do actually notice this from time to time, otherwise I wouldn't have posted this) So, as said, if it's trivial to fix by swapping items in the `SlideFilterModel : QSortFilterProxyModel` instead of increasing the index by one, or maybe just reversing the random order, then there's no reason not to do that. If I was more familiar with the code, I'd submit a MR, but unfortunately, I am not. Thanks for your consideration.
Sending along for consideration
The code for random wallpaper slideshows is already very complex; surprisingly so, even. I think adding additional complexity for something quite minor is unfortunately not worth the risk. Thanks for the idea anyway, though!
(In reply to Nate Graham from comment #4) > The code for random wallpaper slideshows is already very complex; > surprisingly so, even. I think adding additional complexity for something > quite minor is unfortunately not worth the risk. Thanks for the idea anyway, > though! Would you still accept a merge request which adds a very simple `SlideFilterModel::reverse()` for reversing the `QList<int> m_randomOrder` instead of the `m_currentSlide += 1` increment? I don't think this is very complex at all. $ git diff --shortstat wallpapers/ 3 files changed, 9 insertions(+), 1 deletion(-) I tested this with added debug log messages and it's working fine. If you don't want a MR for this, then I can keep this patch in my own plasma-workspace builds, but I don't see a reason not to upstream this.
(In reply to bastimeyer123 from comment #5) > Would you still accept a merge request which adds a very simple > `SlideFilterModel::reverse()` for reversing the `QList<int> m_randomOrder` > instead of the `m_currentSlide += 1` increment? I don't think this is very > complex at all. The best thing to do would be to create the MR and let the development community review it. They don't read individual bug reports for patch suggestions. The worst that can happen is it isn't accepted. Since it's a small change, they might be open to it.
If you have a simple patch that you think doesn't present risk, I'd agree that you can go ahead and submit it, and we'll see where it goes. Make sure to put "BUG: 509711" on its own line somewhere in the commit message, so this bug report changes from RESOLVED INTENTIONAL to RESOLVED FIXED if and when the patch gets merged.