D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

This revision was automatically updated to reflect the committed changes. Closed by commit R120:ea32a7611227: [Image Wallpaper Slideshow] Allow setting of different sorting orders (authored by davidre). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D22121?vs=62529=62530#toc REPOSITORY

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 62529. davidre added a comment. 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 REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. How can we move this forward? Are there things I overlooked? Is it sill crashing sometimes? Code comments? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma, davidedmundson Cc: davidedmundson, msdobrescu,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. I no longer have any crashes with this latest version! \o/ REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma, davidedmundson Cc: davidedmundson, msdobrescu, ngraham, filipf, plasma-devel, LeGast00n,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61648. davidre added a comment. - 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. Could it be some threading issue? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma, davidedmundson Cc: davidedmundson, msdobrescu, ngraham, filipf, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre planned changes to this revision. davidre added a comment. I may have introduced a similar crash or made it easier to trigger Nate's crash. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma, davidedmundson Cc:

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61335. davidre added a comment. - Remember current slide between starts REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22121?vs=61318=61335 BRANCH slideshow (branched from master) REVISION DETAIL

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. In D22121#492179 , @msdobrescu wrote: > 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. In D22121#492173 , @msdobrescu wrote: > 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. 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? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61318. davidre added a comment. 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61314. davidre marked 2 inline comments as done. davidre added a comment. 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. 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? REPOSITORY R120 Plasma

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. IMHO, for lines like these, 'm_currentSlide = m_slideFilterModel->indexOf(path) - 1;', checks must be added because it is transparent to the QML code and let room for failure. Besides that, did you know that if there are no wallpapers, could be added one or

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. In D22121#491833 , @davidedmundson wrote: > 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. Indeed, at should crash, but when is that string empty? I can't crash mine, although inherits the same code. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma, davidedmundson Cc: davidedmundson,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidedmundson requested changes to this revision. davidedmundson added a comment. This revision now requires changes to proceed. 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. Ha Ha Ha! I know! I have asked you about, remember? Sometimes the actual build goes bananas and needs a reset. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma Cc: davidedmundson, msdobrescu, ngraham,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. In D22121#491723 , @msdobrescu wrote: > In D22121#491369 , @ngraham wrote: > > > Huh, in a VM everything works for me too. I wonder if there's some subtle issue with the

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

msdobrescu added a comment. In D22121#491369 , @ngraham wrote: > 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.

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added inline comments. INLINE COMMENTS > image.cpp:600 > +m_slideshowModel->addBackground(path); > +m_currentSlide = m_slideFilterModel->indexOf(path) - 1; > nextSlide(); Currently the slideshow would restart. I guess we could check here if the index is -1. Or

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidedmundson added inline comments. INLINE COMMENTS > image.cpp:600 > +m_slideshowModel->addBackground(path); > +m_currentSlide = m_slideFilterModel->indexOf(path) - 1; > nextSlide(); what will happen if you add a background that the slideFilterModel then filters

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. 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. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To:

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. In D22121#490983 , @ngraham wrote: > 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. REPOSITORY R120 Plasma Workspace REVISION

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. I still get the same crash. :( REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma Cc: msdobrescu, ngraham, filipf, plasma-devel, LeGast00n, jraleigh, fbampaloukas, GB_2, ragreen, Pitel, ZrenBot, himcesjf,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61116. davidre added a comment. - Sort case insensitive REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22121?vs=61115=61116 BRANCH slideshow (branched from master) REVISION DETAIL

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61115. davidre added a comment. 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. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. Weird. I can't get it to crash at all but I will look at and think about it. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma Cc: msdobrescu, ngraham, filipf, plasma-devel, LeGast00n, jraleigh,

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. Now Plasma crashes when I click Apply no matter which mode is selected: 0x74ac682f in raise () from /usr/lib/libc.so.6 (gdb) bt #0 0x74ac682f in raise () at /usr/lib/libc.so.6 #1 0x74ab1672 in abort () at /usr/lib/libc.so.6

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61081. davidre added a comment. - Bool is now other way around REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22121?vs=61078=61081 BRANCH slideshow (branched from master) REVISION DETAIL

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 61078. davidre added a comment. - Don't invalidate if we are in the config view and the mode is set to random REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22121?vs=60826=61078 BRANCH slideshow (branched

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. 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. REPOSITORY R120 Plasma

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. 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? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D22121 To: davidre, #plasma

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre added a comment. In D22121#488479 , @ngraham wrote: > 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. 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

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 60826. davidre added a comment. - add my copyright REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22121?vs=60825=60826 BRANCH slideshow (branched from master) REVISION DETAIL

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 60825. davidre marked 4 inline comments as done. davidre added a comment. - String changes and remove debug statement REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D22121?vs=60823=60825 BRANCH slideshow

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre updated this revision to Diff 60823. davidre added a comment. 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. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

ngraham added a comment. +100 Will do a bigger review and test tomorrow! INLINE COMMENTS > config.qml:147 > +{ > +'label': i18nd("plasma_wallpaper_org.kde.image", > "Modification time (oldest first"), > +'slideshowMode':

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

filipf added a comment. Awesome  INLINE COMMENTS > config.qml:135 > +{ > +'label': i18nd("plasma_wallpaper_org.kde.image", > "Alphabetical (A first)"), > +'slideshowMode': Wallpaper.Image.Alphabetical Instead of "A first" I think we

D22121: [Image Wallpaper Slideshow] Allow setting of different sorting orders

davidre created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidre requested review of this revision. REVISION SUMMARY Allows setting of other sorting orders like alphabetical or last modified date. To enable this a new ProxyModel is introduced