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

Entities/parameter values deleted from DB still visible #2295

Closed
PiispaH opened this issue Sep 1, 2023 · 12 comments
Closed

Entities/parameter values deleted from DB still visible #2295

PiispaH opened this issue Sep 1, 2023 · 12 comments
Labels
bug Something isn't working
Milestone

Comments

@PiispaH
Copy link
Collaborator

PiispaH commented Sep 1, 2023

  • Open a DB in DB editor and add some entities and parameter definitions/values
  • Commit the changes
  • Delete some of the previously added entities or parameter values -> they will still show up in the DB editor
  • Commit the DB -> deleted items still visible
  • Press the reload button -> deleted items vanish
@PiispaH PiispaH added bug Something isn't working 0.8-dev labels Sep 1, 2023
@PiispaH PiispaH added this to the v0.8.0 milestone Sep 1, 2023
@soininen soininen self-assigned this Sep 1, 2023
@soininen
Copy link
Contributor

soininen commented Sep 1, 2023

I ran into similar issue while fixing unit tests under #2294.

@manuelma
Copy link
Collaborator

manuelma commented Sep 1, 2023

Thanks for reporting and please let me know if you need help understanding the new code. Committing has changed considerably so I am not surprised to see something like this...

@soininen
Copy link
Contributor

soininen commented Sep 1, 2023

It's about a TempId not being resolved to real id in Toolbox side. Using TempIdDict in entity tree items fixes the issue. I need to still figure out why the corresponding unit test fails - perhaps it needs to wait for the pending changes to take place. The unit test is fixed now as well.

soininen added a commit that referenced this issue Sep 1, 2023
Resolution of temporary ids to real ids needs to reach the Database editor,
too. This fixes an issue where you couldn't delete an added item if the
addition was committed to the database in the same session.

Re #2294,#2295
@soininen
Copy link
Contributor

soininen commented Sep 1, 2023

Hopefully the above commit fixes this.

@soininen soininen closed this as completed Sep 1, 2023
@manuelma
Copy link
Collaborator

manuelma commented Sep 4, 2023

Very nice catch @soininen

I am thinking that something like this can also affect the parameter tables and maybe the pivot table, and basically every Qt model where we store DB ids. It makes me wonder... The TempId approach is nice in the sense it removes the need to access the DB for creating new items, but at the same time it might be a bit too crazy.

What worries me is clients will need to import and use this TempIdDict guy everytime they'd want to develop something that stores DB ids... Maybe we need to think about this more... Maybe I can present the issue in the next dev meeting and we can all brainstorm...

@soininen
Copy link
Contributor

soininen commented Sep 4, 2023

@manuelma How about we never change the client-side id, not even when the item is committed? The db map could store the real id in the cache item or keep a lookup table on client id <--> real id transformations. This way we wouldn't need to notify clients at all. Well, that's just one idea. In any case, I'm still undecided what to think about the TempIdDict stuff.

What I'm sure of is that we should stop hijacking this issue's discussion :)

@PiispaH
Copy link
Collaborator Author

PiispaH commented Sep 4, 2023

This issue still seems to persist for me if the db editor session is not refreshed between adding the items and deleting them.

@PiispaH PiispaH reopened this Sep 4, 2023
@soininen
Copy link
Contributor

soininen commented Sep 4, 2023

Looks like I fixed this only for the Entity tree and not for Parameter value table.

@manuelma
Copy link
Collaborator

manuelma commented Sep 4, 2023

Every Qt model must be affected by this, because we store DB ids in their internal data structure. We need to use a TempIdDict everywhere instead (and I don't know how will that work for the parameter tables where the id is stored in a list rather than a dict). I like @soininen 's idea about never resolving ids and instead keeping a mapping from real-id to temp-id somewhere in DatabaseMapping or DBCacheBase...?

@manuelma
Copy link
Collaborator

manuelma commented Sep 5, 2023

I would like to give this one a try @soininen @PiispaH - I believe there is an easy way to implement @soininen 's idea and get rid of the TempIdDict.

@soininen soininen removed their assignment Sep 5, 2023
@soininen
Copy link
Contributor

soininen commented Sep 5, 2023

@manuelma Sure, please go ahead! I got distracted and am currently bogged down with some other stuff.

manuelma added a commit that referenced this issue Sep 5, 2023
@manuelma
Copy link
Collaborator

manuelma commented Sep 5, 2023

I think it got solved, will close again...

@manuelma manuelma closed this as completed Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants