Bug 387824 - gwenview fails PlaceTreeModelTest autotest with frameworks 5.41
Summary: gwenview fails PlaceTreeModelTest autotest with frameworks 5.41
Status: RESOLVED FIXED
Alias: None
Product: gwenview
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: unspecified Linux
: NOR normal
Target Milestone: ---
Assignee: Gwenview Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-12 10:33 UTC by Rik Mills
Modified: 2018-04-07 11:55 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Rik Mills 2017-12-12 10:33:15 UTC
On KDE CI for stable and master branches, gwenview fails it's autotests:

https://build.kde.org/job/Applications%20gwenview%20stable-kf5-qt5%20SUSEQt5.9/8/testReport/(root)/TestSuite/placetreemodeltest/

https://build.kde.org/job/Applications%20gwenview%20kf5-qt5%20SUSEQt5.9/10/testReport/(root)/TestSuite/placetreemodeltest/

with

FAIL!  : PlaceTreeModelTest::testListPlaces() Compared values are not the same
   Actual   (model.rowCount()): 10
   Expected (2)               : 2
   Loc: [/home/jenkins/workspace/Applications gwenview stable-kf5-qt5 SUSEQt5.9/tests/auto/placetreemodeltest.cpp(126)]

The failure is also seen as a fresh regression in 17.08.3 version when testing against new Frameworks 5.4.1 in ubuntu autopackage tests, so it seems that frameworks is the issue or cause (whether or not it needs to be fixed there or in gwenview itself)

e.g. https://objectstorage.prodstack4-5.canonical.com/v1/AUTH_77e2ada1e7a84929a74ba3b87153c0ac/autopkgtest-bionic/bionic/amd64/g/gwenview/20171212_081612_d79ac@/log.gz

The first failure on the KDE CI is: 

28-Nov-2017 23:00

whereas

28-Nov-2017 05:41

passes.

This suggests a change in frameworks in that timescale to be the 'cause'.
Comment 1 null 2017-12-14 17:28:37 UTC
Already noticed it working locally but not on the CI too, but didn't have time to look into it yet. Having a bug increased priority, though ;)

Thanks for pointing out the connection to KF 5.41. Some digging and bisecting shows this is caused by 7eb6333bdb48 in KIO ("Added baloo urls into places model"), i.e. https://phabricator.kde.org/D8332.

Besides the test, this results in new entries in the Places tab on the Start Page:
- 4 entries for "Recently Saved" (which seem sensible)
- 4 entries for "Search For" (where Images and Videos make sense, but Documents and Audio Files not so much – will D9332 help with this?)

Unfortunately, besides showing those entries, Gwenview stumbles a bit when accessing the virtual folders:
- Error messages when tree-expanding those from the sidebar.
- Access via Places panel works, but not via folders sidebar.
- ASSERT: "subjobs().isEmpty()" and coredump when quickly switching back and forth between search results and Start Page.

New usability issues, too:
- Need to scroll to access "Devices" in Places panel (viewing images on external devices is an important feature). Perhaps make the icon size configurable and the default size as small as in Dolphin?
- 8 more entries in the folders sidebar, making it too much for efficient visual orientation. Either group all timeline related entries or replicate the entire grouping from Places panel.
- Recent Folders tab on the Start Page shows long and unreadable baloosearch:/ URLs with percent encoding.

To make things worse, simply reverting to 5.40 does not work, because the entries are now saved in .local/share/user-places.xbel permanently.

In general Gwenview should not show broken functionality, so one of the following things need to be done (haven't looked at the code yet, not sure whether this is simple or not):
- Block those entries completely and also handle users who used 5.41 before Gwenview could ship such a fix.
- Fix Gwenview to handle these entries properly and only show those that make sense. ← Preferred solution.
- (I guess the patch in KIO itself is fine, although a bit risky wrt. expectations for the apps.)

Once that's done, we'll know the magic number to be able to fix the failing test properly. However, it is currently unclear who would be willing to work on those items. I'll be availabe to review and test, at least.
Comment 2 Nate Graham 2017-12-14 19:32:12 UTC
CCing Renato. Would be great to get a patch that fix some of the above-mentioned issues.


- Recent Folders tab on the Start Page shows long and unreadable baloosearch:/ URLs with percent encoding.

May be related to or the same issue as Bug 383850.
Comment 3 null 2017-12-14 19:39:11 UTC
>> - Recent Folders tab on the Start Page shows long and unreadable 
>> baloosearch:/ URLs with percent encoding.
> May be related to or the same issue as Bug 383850.
Not really, see https://en.wikipedia.org/wiki/Percent_encoding.
Comment 4 Nate Graham 2017-12-14 19:43:22 UTC
Yes, but I was referring to the fact that we see "baloosearch://<some ugly string>" in the first place (rather than "<human-readable string>"). That's what seems like it could be the same or a similar issue. I could be wrong though.
Comment 5 null 2017-12-14 19:46:33 UTC
You are not wrong, but the fix for the other bug is probably some simple function call, while the representation of a search URL requires much more thinking/designing/coding to get right.

Anyway, that's hardly the most important issue here.
Comment 6 Renato Araujo Oliveira Filho 2017-12-14 19:54:33 UTC
(In reply to Nate Graham from comment #2)
> CCing Renato. Would be great to get a patch that fix some of the
> above-mentioned issues.
> 
> 
> - Recent Folders tab on the Start Page shows long and unreadable
> baloosearch:/ URLs with percent encoding.
> 
> May be related to or the same issue as Bug 383850.

Yes this is expected, the baloo ulr is not human readable. Dolphin has special cases for that and hide. We can probably hide it too on file dialog but this is not a bug.
Comment 7 Renato Araujo Oliveira Filho 2017-12-15 15:26:05 UTC
I have this patch to fix the problem with side panel: https://phabricator.kde.org/D9351
Comment 8 null 2017-12-15 21:06:17 UTC
> - 8 more entries in the folders sidebar, making it too much for efficient
> visual orientation. Either group all timeline related entries or replicate the
> entire grouping from Places panel.
To expand on this usability problem: Currently it is not clear that the time related entries are about "Recently Saved" items and thus could easily be confused with similar functions of other photo managers, i.e. automatic categorization based on the metadata's timestamp.
Comment 9 null 2017-12-21 00:08:56 UTC
Git commit 50e6fa3ffc490eca33b8e2025120ec041b333fee by Henrik Fehlauer.
Committed on 20/12/2017 at 23:57.
Pushed by rkflx into branch 'Applications/17.12'.

Fix expanding of baloo URLs in folders sidebar tree view

D8332 resulted in baloo URLs like `timeline:/yesterday` appearing in
Gwenview, however accessing those in the {nav Folders} sidebar led to
error dialogs. 86d754546bd1 already solved this for directly clicking on
entries, here we apply the same fix for expanding those entries, i.e.
clicking on the small arrows in the tree view.

M  +4    -0    lib/placetreemodel.cpp

https://commits.kde.org/gwenview/50e6fa3ffc490eca33b8e2025120ec041b333fee
Comment 10 null 2018-01-07 23:48:13 UTC
Git commit e8d799c9b522af299f721dbbfcc650526ba78940 by Henrik Fehlauer.
Committed on 07/01/2018 at 23:47.
Pushed by rkflx into branch 'Applications/17.12'.

Fix failing PlaceTreeModelTest autotest

Since Frameworks 5.41, `PlaceTreeModelTest` would fail. Bisecting shows
this has been caused by 7eb6333bdb48 in KIO ("Added baloo urls into
places model"), where the model now returns 4 additional entries for
"Recently Saved" and another 4 entries for "Search For".

While the fix is trivially done by changing the number of expected
items, in a way the test functioned as a canary for problems deeper in
the code. In particular it uncovered problems when accessing the newly
added virtual folders as well as various usability issues. Some of those
are fixed already (50e6fa3ffc49 and 86d754546bd1), but there is still
some work left to do.

Test Plan: `placetreemodeltest` does not fail with KF 5.41 anymore,
still works with KF 5.40.

M  +6    -0    tests/auto/placetreemodeltest.cpp

https://commits.kde.org/gwenview/e8d799c9b522af299f721dbbfcc650526ba78940
Comment 11 null 2018-01-07 23:51:23 UTC
I changed the test for now to handle the different number of items Gwenview gets from the PlaceTreeModel since KIO 5.41, so at least we make the CI a bit happier for 17.12.1.

Any ideas how/where to track the remaining issues?