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

Recover branch selection #196

Conversation

overengineer
Copy link
Contributor

@overengineer overengineer commented May 16, 2024

When user refreshes the stream, branch selection resets. As a workaround, I stored selected branch in a class member. Then used that value to get active branch. Better solutions are possible of course.

@overengineer overengineer changed the title Recover selected branch Recover branch selection May 16, 2024
@overengineer overengineer force-pushed the fix/resetting_selected_branch_workaround branch from 7b3c161 to 5e01d5a Compare May 16, 2024 15:19
@overengineer overengineer marked this pull request as ready for review May 16, 2024 15:20
@JR-Morgan
Copy link
Member

JR-Morgan commented May 24, 2024

I haven't had a deep look at your PR just yet @overengineer
I'll make this a priority for next week.

Not sure if it relates to this PR. But I just thought I'd share my thoughts regardless.

In other connectors, we have tried to use the account id / stream id / branch id / commit id as the key for the selection instead of a sequential index.

I haven't investigated how possible this is, but we do look to be setting a Key here with str(i) (and similar elsewhere for accounts, commits, etc.)

This concept could also fix #98

@overengineer
Copy link
Contributor Author

overengineer commented May 30, 2024

Hi @JR-Morgan
I updated my changes. Now I am matching objects by IDs.
Also I am checking if user and stream changed before restoring branch and commit selections.

I used hooks to listen user actions. because I could not find a way to access selection states.
BTW, I had to use those class members because all scene objects were resetting on update user.

I would prefer putting the code for selection states to a seperate file. But I hesitated because I do not know your folder structure conventions. If you have a suggestion, I can refactor.

Copy link
Member

@JR-Morgan JR-Morgan left a comment

Choose a reason for hiding this comment

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

Looking good, just a few comments

bpy_speckle/properties/scene.py Outdated Show resolved Hide resolved
bpy_speckle/properties/scene.py Outdated Show resolved Hide resolved
bpy_speckle/properties/scene.py Outdated Show resolved Hide resolved
@overengineer
Copy link
Contributor Author

@JR-Morgan
I made some changes according to your comments.

Also I converted class members to object members and instantiated the class. This way is less ugly I think.

@JR-Morgan JR-Morgan merged commit 230e27a into specklesystems:main Jun 5, 2024
1 check passed
@JR-Morgan
Copy link
Member

Thanks @overengineer for your patience,
I've made a couple tiny tweaks to the type hinting, and merged.

Thanks for you contribution!

@JR-Morgan
Copy link
Member

Change is now in 2.20.0-wip

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.

2 participants