[Image Wallpaper Slideshow] Allow setting of different sorting orders
ClosedPublic

Authored by davidre on Jun 27 2019, 1:05 PM.

Details

Summary

Allows setting of other sorting orders like alphabetical or last modified date.
To enable this a new ProxyModel is introduced which handles the sorting and
filtering (as indicated by the checkboxes). This is backed by the slideshowModel
whcih previously as only used for the configutation. The lists of slides and unseen
slides are dropped as now the slides that are shown are taken from the model.

FEATURE: 186181
FIXED-IN: 5.17.0

Test Plan




Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davidre created this revision.Jun 27 2019, 1:05 PM
Restricted Application added a project: Plasma. Β· View Herald TranscriptJun 27 2019, 1:05 PM
Restricted Application added a subscriber: plasma-devel. Β· View Herald Transcript
davidre requested review of this revision.Jun 27 2019, 1:05 PM
davidre edited the test plan for this revision. (Show Details)Jun 27 2019, 1:08 PM
davidre added a reviewer: Plasma.
filipf added a subscriber: filipf.Jun 27 2019, 4:07 PM

Awesome 😍

wallpapers/image/imagepackage/contents/ui/config.qml
136

Instead of "A first" I think we can just say "A to Z" or even "A-Z"

140

Same comment as above just reverse A and Z.

144

Something like "Date modified" might be closer to what we have in file managers.

+100

Will do a bigger review and test tomorrow!

wallpapers/image/imagepackage/contents/ui/config.qml
148

Needs a trailing ")" in the label

davidre edited the summary of this revision. (Show Details)Jun 28 2019, 6:47 PM
davidre updated this revision to Diff 60823.Jun 29 2019, 9:49 AM

Sort only if a sortmode has been specified

Otherwise it could have been initialized to an invalid value which caused us to
reach Q_UNREACHABLE and crash.

davidre updated this revision to Diff 60825.Jun 29 2019, 9:53 AM
davidre marked 4 inline comments as done.
  • String changes and remove debug statement
davidre updated this revision to Diff 60826.Jun 29 2019, 9:59 AM
  • add my copyright

That fixed my crash, yay!

I see a UX issues with the Random mode though: since the list re-sorts itself randomly, it becomes harder to locale any given wallpaper in there (say, for the purposes of unchecking it). Also, when you scroll all the way to the bottom and then back up to the top, the order has changed to a new random order. While you're scrolling, the delegates are visible changing before your eyes. It's all a bit odd.

I wonder if in the Random mode, we should just keep the view sorted alphabetically?

That fixed my crash, yay!

I see a UX issues with the Random mode though: since the list re-sorts itself randomly, it becomes harder to locale any given wallpaper in there (say, for the purposes of unchecking it). Also, when you scroll all the way to the bottom and then back up to the top, the order has changed to a new random order. While you're scrolling, the delegates are visible changing before your eyes. It's all a bit odd.

I wonder if in the Random mode, we should just keep the view sorted alphabetically?

Yes I had the same thoughts about random. Another way would be to not resort the slides at all in the config view when you change it and alphabetically/how we get them when you open the config view. Either way I guess I will have to change filterUncheckedSlides to a generic isUsedinConfig to enable that.

Yeah maybe we should always show them in alphabetical order, or maybe we should show them in alphabetical order just for the alphabetical and random modes?

Ah I forgot a word there.
I wanted to say don't change the current order if the sort order is switched to random. And when opened and the current mode is random whatever.
Displaying the order for the other modes is quite good I think.

davidre updated this revision to Diff 61078.Jul 3 2019, 3:01 PM
  • Don't invalidate if we are in the config view and the mode is set to random
davidre updated this revision to Diff 61081.Jul 3 2019, 3:24 PM
  • Bool is now other way around

Now Plasma crashes when I click Apply no matter which mode is selected:

0x00007ffff4ac682f in raise () from /usr/lib/libc.so.6
(gdb) bt
#0  0x00007ffff4ac682f in raise () at /usr/lib/libc.so.6
#1  0x00007ffff4ab1672 in abort () at /usr/lib/libc.so.6
#2  0x00007ffff4fc87fc in  () at /usr/lib/libQt5Core.so.5
#3  0x00007ffff4fc7c28 in qt_assert_x(char const*, char const*, char const*, int) ()
    at /usr/lib/libQt5Core.so.5
#4  0x00007fffdc35fb91 in QString::at(int) const (this=0x7fffffff26c8, i=-1)
    at /usr/include/qt/QtCore/qstring.h:936
#5  0x00007fffdc35cb55 in BackgroundListModel::indexOf(QString const&) const
    (this=0x555555f1d8d0, path=...)
    at /home/nate/kde/src/plasma-workspace/wallpapers/image/backgroundlistmodel.cpp:238
#6  0x00007fffdc354654 in Image::setWallpaper(QString const&)
    (this=0x555555eff620, path=...)
    at /home/nate/kde/src/plasma-workspace/wallpapers/image/image.cpp:600
#7  0x00007fffdc3543f2 in Image::addUrl(QUrl const&, bool)
    (this=0x555555eff620, url=..., setAsCurrent=true)
    at /home/nate/kde/src/plasma-workspace/wallpapers/image/image.cpp:566
#8  0x00007fffdc352003 in Image::addUrl(QString const&) (this=0x555555eff620, url=...)
    at /home/nate/kde/src/plasma-workspace/wallpapers/image/image.cpp:126
#9  0x00007fffdc34d3a6 in Image::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (_o=0x555555eff620, _c=QMetaObject::InvokeMetaMethod, _id=30, _a=0x7fffffff2b00)
    at /home/nate/kde/build/plasma-workspace/wallpapers/image/plasma_wallpaper_imageplugin_autogen/EWIEGA46WW/moc_image.cpp:327
#10 0x00007fffdc34df6d in Image::qt_metacall(QMetaObject::Call, int, void**)
    (this=0x555555eff620, _c=QMetaObject::InvokeMetaMethod, _id=30, _a=0x7fffffff2b00)
    at /home/nate/kde/build/plasma-workspace/wallpapers/image/plasma_wallpaper_imageplugin_autogen/EWIEGA46WW/moc_image.cpp:525
#11 0x00007ffff705a62e in  () at /usr/lib/libQt5Qml.so.5
#12 0x00007ffff6f5b592 in  () at /usr/lib/libQt5Qml.so.5
#13 0x00007ffff6f5d0a5 in  () at /usr/lib/libQt5Qml.so.5
#14 0x00007ffff6f5e19a in QV4::QObjectMethod::callInternal(QV4::Value const*, QV4::Value const*, int) const () at /usr/lib/libQt5Qml.so.5
#15 0x00007ffff6f7953a in  () at /usr/lib/libQt5Qml.so.5
#16 0x00007ffff6f7c60f in  () at /usr/lib/libQt5Qml.so.5
#17 0x00007ffff6f09f74 in QV4::Function::call(QV4::Value const*, QV4::Value const*, int, QV4::ExecutionContext const*) () at /usr/lib/libQt5Qml.so.5
#18 0x00007ffff7082978 in QQmlJavaScriptExpression::evaluate(QV4::CallData*, bool*) ()
    at /usr/lib/libQt5Qml.so.5
#19 0x00007ffff70264f8 in QQmlBoundSignalExpression::evaluate(void**) ()
    at /usr/lib/libQt5Qml.so.5
#20 0x00007ffff70276ac in  () at /usr/lib/libQt5Qml.so.5
#21 0x00007ffff7065173 in QQmlNotifier::emitNotify(QQmlNotifierEndpoint*, void**) ()
    at /usr/lib/libQt5Qml.so.5
#22 0x00007ffff70087f4 in QQmlData::signalEmitted(QAbstractDeclarativeData*, QObject*, int, void**) () at /usr/lib/libQt5Qml.so.5
#23 0x00007ffff51e7aff in QMetaObject::activate(QObject*, int, int, void**) ()
    at /usr/lib/libQt5Core.so.5
#24 0x00007ffff70044b6 in QQmlVMEMetaObject::metaCall(QObject*, QMetaObject::Call, int, void**) () at /usr/lib/libQt5Qml.so.5
#25 0x00007ffff708c9f9 in  () at /usr/lib/libQt5Qml.so.5
#26 0x00007ffff708d1b8 in  () at /usr/lib/libQt5Qml.so.5

Weird. I can't get it to crash at all but I will look at and think about it.

davidre updated this revision to Diff 61115.Jul 4 2019, 9:24 AM

I couldm't crash it by clicking apply but noticed some type errors because the proxymodel
din't have all the functionality of the underlying model.

davidre updated this revision to Diff 61116.Jul 4 2019, 9:34 AM
  • Sort case insensitive

I still get the same crash. :(

I still get the same crash. :(

Very weird I just tried it in a VM which hadn't seen the patch at all yet and it doesn't crash.

Huh, in a VM everything works for me too. I wonder if there's some subtle issue with the fact that I'm running a custom-compiled Plasma at a different path on bare metal.

davidedmundson added inline comments.
wallpapers/image/image.cpp
600

what will happen if you add a background that the slideFilterModel then filters out?

davidre added inline comments.Jul 6 2019, 7:12 PM
wallpapers/image/image.cpp
600

Currently the slideshow would restart. I guess we could check here if the index is -1. Or alternatively don't trigger this from the qml side if the image is unchecked. In my mind this even better because then we enable the apply button, too.

Huh, in a VM everything works for me too. I wonder if there's some subtle issue with the fact that I'm running a custom-compiled Plasma at a different path on bare metal.

Hi, I have followed this one, the new version, step by step: https://community.kde.org/Get_Involved/development#Plasma
Before, some custom script was necessary, but did not work and all my crashes, I have experienced and told you about, were due to that. Could be some environment variable that pointed to something from the main install.
I recommend to perform a full cleanup, follow again the testing setup, rebuild all the deps (kdesrc-build plasma-desktop plasma-workspace plasma-framework plasma-nm plasma-pa plasma-thunderbolt plasma-vault plasma-workspace-wallpapers kdeplasma-addons kwin systemsettings kscreen breeze discover kinfocenter --include-dependencies ~ and maybe add ~ --refresh-build), then test and see if it works. This was the way I've got rid of the errors that should not come in the first place, before developing and correcting it for real.

Huh, in a VM everything works for me too. I wonder if there's some subtle issue with the fact that I'm running a custom-compiled Plasma at a different path on bare metal.

Hi, I have followed this one, the new version, step by step: https://community.kde.org/Get_Involved/development#Plasma
Before, some custom script was necessary, but did not work and all my crashes, I have experienced and told you about, were due to that. Could be some environment variable that pointed to something from the main install.
I recommend to perform a full cleanup, follow again the testing setup, rebuild all the deps (kdesrc-build plasma-desktop plasma-workspace plasma-framework plasma-nm plasma-pa plasma-thunderbolt plasma-vault plasma-workspace-wallpapers kdeplasma-addons kwin systemsettings kscreen breeze discover kinfocenter --include-dependencies ~ and maybe add ~ --refresh-build), then test and see if it works. This was the way I've got rid of the errors that should not come in the first place, before developing and correcting it for real.

Yes, that's how I do it. In fact, I'm one who wrote that part of the documentation. :)

Ha Ha Ha! I know! I have asked you about, remember?
Sometimes the actual build goes bananas and needs a reset.

davidedmundson requested changes to this revision.Jul 7 2019, 2:09 PM

Nate's crash is due to data not code.

for (int i = 0; i < m_packages.size(); i++) {
    QString package = m_packages[i].path();
    if (package.at(package.length() - 1) == QChar::fromLatin1('/')) {   <--- you know you crash on this line

we're calling .at(length -1) unconditionally

For an empty string our index will be -1.

This hints towards an insertion bug rather than saying we should just add a guard here.

This revision now requires changes to proceed.Jul 7 2019, 2:09 PM

Indeed, at should crash, but when is that string empty?
I can't crash mine, although inherits the same code.

Nate's crash is due to data not code.

for (int i = 0; i < m_packages.size(); i++) {
    QString package = m_packages[i].path();
    if (package.at(package.length() - 1) == QChar::fromLatin1('/')) {   <--- you know you crash on this line

we're calling .at(length -1) unconditionally

For an empty string our index will be -1.

This hints towards an insertion bug rather than saying we should just add a guard here.

Thanks for the hint David. @ngraham could you maybe try to get the path for which the crash happens (if package.length == 0) and how the folder structure for that image is added to your slideshow. Also it would be good to know if it happens if you start fresh and add it in the same way (then I could reproduce it) or not.

msdobrescu added a comment.EditedJul 7 2019, 4:36 PM

Besides that, did you know that if there are no wallpapers, could be added one or more by drag and drop?
Image files could be added by drag and drop, but never appear, although they are in the list, where should be folders only IMHO.
They stay there, nothing happens, no background change on the desktop.
Also, if one folder is added, 'Apply' button pressed, then removed, after 'Apply', the background remains set to the last image, although I would have expected to have no image set as wallpaper.

For long folder names, the removal and open folder icons are mixed with the folder name in an ... ugly way... It's almost illegible.

wallpapers/image/image.cpp
600

IMHO, here must be checked because it is transparent to the QML code and lets room for failure.

Is it more efficient from the performance point of view to have a QSortFilterProxyModel class per sorting method? I have a 6000 images case, would that be consuming compared to setting the sort class once according to the sorting method?

davidre updated this revision to Diff 61314.Jul 8 2019, 8:50 AM
davidre marked 2 inline comments as done.

Disable selecting the wallpaper in slideshow configuration

This was introduced when the gridview from the single image dialog was added to the
slideshow one. In my mind it makes no sense to have it, by clicking on a image the user
doesn't change any visible setting and needlesly changes changes the state of the configuration
dialog.

davidre updated this revision to Diff 61318.Jul 8 2019, 9:13 AM

Don't start if we are in config mode

I had another crash here via reload -> processPaths -> endInsertRows.
Triggered by repeatetly checking and unchecking a checkbox. My guess is that because of multiple
calls to reload and beginInsertRows, endInsertRows there were some inconsistencies. With this
I can't crash it anymore by checking and unchecking a checkbox.

I'd say it is some half-implemented feature, probably. Could it be useful to navigate by keyboard and check/uncheck images, by using space, for example?

I'd say it is some half-implemented feature, probably. Could it be useful to navigate by keyboard and check/uncheck images, by using space, for example?

You can still navigate by Keyboard. But selecting an image will not enable the apply button anymore and then when clicked set the current image to the one selected.
I like your idea that you can uncheck/check when the whole delegate is selected.

Thanks, selecting from any image is useful, to avoid starting always from the top one. But should not enable the button until a check is done.

Thanks, selecting from any image is useful, to avoid starting always from the top one. But should not enable the button until a check is done.

Ideally if you want a specific image you should use a static wallpaper. But I see what you mean, I think it would be useful to remember the current slide between restarts.

davidre updated this revision to Diff 61335.Jul 8 2019, 12:20 PM
  • Remember current slide between starts
davidre planned changes to this revision.Jul 8 2019, 2:41 PM

I may have introduced a similar crash or made it easier to trigger Nate's crash.

So I'm crashing when I hit apply and then ok because addURL is called from qml with an empty string and I don't understand why. In main.qml I have

onConfiguredImageChanged: {
      if (modelImage != configuredImage) {
          console.log("configured Image" + wallpaper.configuration.Image);
          console.log(configuredImage + "!=" + modelImage)
          imageWallpaper.addUrl(configuredImage);
      }
  }
  onModelImageChanged:{
      Qt.callLater(loadImage);
      console.log("mic to "+modelImage);
      wallpaper.configuration.Image = modelImage;
      console.log("mic:" + wallpaper.configuration.Image +" configimage:" +configuredImage);

  }

On apply :

qml: mic to file:///home/david/testimages/hk5lmt9qxfw21.jpg
qml: mic:file:///home/david/testimages/hk5lmt9qxfw21.jpg configimage:file:///home/david/testimages/hk5lmt9qxfw21.jpg

Then clicking on OK

qml: configured Image
qml: !=file:///home/david/testimages/hk5lmt9qxfw21.jpg

What I dont understand is why this happens. When pressing OK without apply everything works as I would expect it.

Could it be some threading issue?

davidre updated this revision to Diff 61648.Jul 12 2019, 12:48 PM
  • Only restore wallpaper on startup
  • Guard against empty path

I couldn't understand why this was being called with an empty string so I just guard
against it at the call site. Maybe this also helps @ngrahams crash but I still don't know
where it is coming from.

I no longer have any crashes with this latest version! \o/

How can we move this forward? Are there things I overlooked? Is it sill crashing sometimes? Code comments?

davidedmundson accepted this revision.Jul 22 2019, 12:38 PM
This revision is now accepted and ready to land.Jul 22 2019, 12:38 PM
ngraham edited the summary of this revision. (Show Details)Jul 22 2019, 2:52 PM
davidre updated this revision to Diff 62529.Jul 25 2019, 10:05 AM

Last touches

  • remove QDebug
  • don't restore image if we are in random mode
  • if we have seen all images generate a new random order
This revision was automatically updated to reflect the committed changes.