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.

Sign up for GitHub

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

Jump to bottom

Memory dataset group and possibility to persist it #37389

Merged
merged 10 commits into from
Jul 1, 2020
Merged

Memory dataset group and possibility to persist it #37389

merged 10 commits into from
Jul 1, 2020

Conversation

vcloarec
Copy link
Member

This PR introduce memory dataset groups for mesh layer. These dataset groups are temporary and are not kept when the project is closed.

Memory dataset groups can be created from the mesh calculator with a new option.

This PR also allows the possibility to remove or save these memory dataset groups to a file with specified driver.

memory

@vcloarec vcloarec added Feature Mesh Related to general mesh layer handling (not specific data formats) labels Jun 24, 2020
@github-actions github-actions bot added this to the 3.16.0 milestone Jun 24, 2020
nyalldawson
nyalldawson reviewed Jun 24, 2020
View reviewed changes
src/app/qgisapp.cpp Outdated
@@ -12854,31 +12856,36 @@ bool QgisApp::checkMemoryLayers()
}
else if ( it.value() && it.value()->isTemporary() )
hasTemporaryLayers = true;
else if ( it.value() && it.value()->type() == QgsMapLayerType::MeshLayer )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this, you should override QgsMapLayer::isTemporary

Copy link
Member Author

Choose a reason for hiding this comment

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

done, but, here I prefer keep the distinguish between mesh layer and other type because the logic is not exactly the same : for other, all the mesh is temporary, for mesh layer, only some dataset groups.

src/app/mesh/qgsmeshdatasetgrouptreeview.cpp Outdated Show resolved Hide resolved
src/app/mesh/qgsmeshdatasetgrouptreeview.cpp Outdated Show resolved Hide resolved
@nyalldawson
Copy link
Collaborator

@nirvn mind doing a UI review of this?

One issue I can see immediately is that the calculator isn't using the correct font in the editor, so we're seeing the qt 5.14 issue with the ridiculous font spacing...

@vcloarec
Copy link
Member Author

One issue I can see immediately is that the calculator isn't using the correct font in the editor, so we're seeing the qt 5.14 issue with the ridiculous font spacing...

In the GIF, It is built with Qt 5.12.8. Maybe is it the space with a variable, for example "vitesse ms". In that case, it is due to the data, the name of the dataset group has the unity at the end, separated with several spaces.

@PeterPetrik
Copy link
Contributor

I have already reviewed this privately in vcloarec#22

@nyalldawson
Copy link
Collaborator

Personally it looks odd to me to show the memory indicator icon in the mesh calculator dialog. Why is it needed there?

@vcloarec
Copy link
Member Author

vcloarec commented Jun 29, 2020 •
edited
Loading

@nyalldawson Memory dataset groups are not persitent and will be removed after closing the project. User could need to know this dataset group is not persistent, especially if this dataset is used to calculate another one. For now, there is no consequences because there is no dependence after the creation of the new one, but there will be soon...

But I agree, it is not the best UI design, especially in the calculator dialog. Maybe it will be better with icon on the right?

@nirvn
Copy link
Contributor

nirvn commented Jun 30, 2020

Please give me some time to review the UI.

One question: why are those virtual datasets have to be non-persistent?

@vcloarec
Copy link
Member Author

One question: why are those virtual datasets have to be non-persistent?

Those "virtual" dataset group are stored in memory, not and file, so can't be retrieved after closing the project.

For information, there is an upcoming PR with "on the fly" dataset group, and those new dataset groups will be calculated on real time, and some (those that not depend on temporary dataset) could be persistent.

@nirvn
Copy link
Contributor

nirvn commented Jun 30, 2020

@vcloarec , couldn't you simply store the formula and re-compute it upon re-opening the project?

@vcloarec
Copy link
Member Author

@vcloarec , couldn't you simply store the formula and re-compute it upon re-opening the project?

The "on the fly" will store the formula and calculate when needed. For the "memory", the formula is not stored, it is not the concept for now. But it could be later... For now, "memory" are just temporary dataset that could be persist in file if needed (as "on the fly")

@nirvn
Copy link
Contributor

nirvn commented Jun 30, 2020

@vcloarec , I'm wondering whether this temporary vs persistent virtual dataset adds a level of UI/UX complexity without much to gain from. I.e., if those virtual datasets would all be persistent, you wouldn't need to have the memory icon anywhere, etc.

@PeterPetrik
Copy link
Contributor

I propose to merge this as it is, and once the on-the-fly PR lands, we can clean the UI in one go?

@nirvn
Copy link
Contributor

nirvn commented Jun 30, 2020

Could we at least remove all memory icons from this PR? That can't stay in any case.

@vcloarec
Copy link
Member Author

vcloarec commented Jun 30, 2020 •
edited
Loading

With the last commit, the memory icon is removed from the dataset active widget and from the mesh calculator.
However, it is still in the available dataset group tree view in the source tab of mesh properties. But I put it on the right of the tree view row.
I think it is necessary to distinguish somewhere dataset group type because "memory" items have not the same behavior (they can be removed or saved), and the user needs to know if there is data that will not be saved if the project is closed.

@nirvn , let me know if it is ok, or do I need to remove ALL?

Here the only memory icon :
image

@PeterPetrik
Copy link
Contributor

talked with @nirvn , agreed on the plan forward, so we can merge this one and improve in the on-the-fly dataset followup PR.

@PeterPetrik PeterPetrik merged commit 6be16a5 into qgis:master Jul 1, 2020
@PeterPetrik PeterPetrik added the Changelog Items that are queued to appear in the visual changelog - remove after harvesting label Jul 1, 2020
@vcloarec vcloarec deleted the registryInLayer branch July 3, 2020 04:50
@vcloarec vcloarec mentioned this pull request Jul 3, 2020
Merged
espinafre pushed a commit to espinafre/QGIS that referenced this pull request Jul 3, 2020
@vcloarec @espinafre
Memory dataset group and possibility to persist it (qgis#37389)
ca44bc3 
[FEATURE] Introduces memory dataset groups for mesh layer. These dataset groups are temporary and are not kept when the project is closed.

Memory dataset groups can be created from the mesh calculator with a new option.

Allows the possibility to remove or save these memory dataset groups to a file with specified driver.
@vcloarec vcloarec mentioned this pull request Jul 5, 2020
Merged
@timlinux timlinux added ChangelogHarvested This PR description has been harvested in the Changelog already. and removed Changelog Items that are queued to appear in the visual changelog - remove after harvesting labels Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nyalldawson nyalldawson nyalldawson left review comments

Assignees
No one assigned
Labels
ChangelogHarvested This PR description has been harvested in the Changelog already. Feature Mesh Related to general mesh layer handling (not specific data formats)
Projects
None yet
Milestone
3.16.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vcloarec @nyalldawson @PeterPetrik @nirvn @timlinux