Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make the SVG selector collapsible (+ remove duplicated code) #39421

Merged
merged 5 commits into from
Oct 21, 2020

Conversation

3nids
Copy link
Member

@3nids 3nids commented Oct 16, 2020

defer loading of SVG icons to only show them when the widget is made visible

this is mainly code moving to its own-class.
this is quite an improvement to me in the general fluidity when working in projects with many SVGs.
it also make the UI much less cluttered.

I'd consider this as a bug fix.

(It takes about 3 seconds to show the widget as the images are loaded, I get a waiting cursor on my OS which is apparently not visible in the video)
svg

defer loading of SVG icons to only show them when the widget is made visible
@3nids 3nids requested a review from nirvn October 16, 2020 08:19
@github-actions github-actions bot added this to the 3.16.0 milestone Oct 16, 2020
@nyalldawson
Copy link
Collaborator

The browser should be loading all the svg previews in a seperate thread. At least it was this way in the past.to avoid any delays with this widget. Is something broken with the thread?

@3nids
Copy link
Member Author

3nids commented Oct 16, 2020

The browser should be loading all the svg previews in a seperate thread.

I realised there were some duplicate code.
Now,

  • I removed the class I created (QgsSvgBrowserWidget)
  • the symbol layer widget is using the dedicated QgsSvgSelector
  • in QgsSvgSelector, there is now a collapsible group box for the tree
  • in QgsAbstractFileContentSourceLineEdit, the tool button is now a QgsPropertyOverrideButton

This basically leads to the same UX than in the screencast, but the freeze is still present.

At least it was this way in the past.to avoid any delays with this widget. Is something broken with the thread?

yes this seems broken.

@3nids 3nids changed the title move SVG browser to a dedicated class make the SVG selector collapsible (+ remove duplicated code) Oct 16, 2020
@3nids
Copy link
Member Author

3nids commented Oct 16, 2020

At least it was this way in the past.to avoid any delays with this widget. Is something broken with the thread

This is actually the initial filling with all SVG icons

// Initially load the icons in the List view without any grouping
QAbstractItemModel *oldModel = mImagesListView->model();
QgsSvgSelectorListModel *m = new QgsSvgSelectorListModel( mImagesListView );
mImagesListView->setModel( m );
delete oldModel; //explicitly delete old model to force any background threads to stop

I replaced the initial model with a path, and there is no blocking anymore.

I would propose to

  1. merge this PR as it's a both a UX improvement (collapsible groupbox) and code duplication removal
  2. discuss the solutions for the loading (do not load all at first, specify a limit, load the images only when scrolling,…)

Copy link
Contributor

@nirvn nirvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggested change. Otherwise I'm good with seeing this merged, good cleanup.

src/gui/symbology/qgssymbollayerwidget.cpp Outdated Show resolved Hide resolved
Co-authored-by: Mathieu Pellerin <nirvn.asia@gmail.com>
@3nids 3nids merged commit 139be61 into qgis:master Oct 21, 2020
@3nids 3nids deleted the svg-browser branch October 21, 2020 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants