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

View Extension API for ViewModel Delegate Commands #9944

Merged
merged 12 commits into from
Sep 2, 2019

Conversation

scottmitchell
Copy link
Collaborator

@scottmitchell scottmitchell commented Aug 27, 2019

Purpose

This PR adds a new class ViewModelCommandExecutive and surfaces this via ViewLoadedParams. The new command executive gives view extensions controlled access to commands on the DynamoViewModel and the current WorkspaceViewModel.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

/// <param name="objectID"></param>
public void FindByIdCommand(string objectID)
{
dynamoViewModel.CurrentSpaceViewModel.FindByIdCommand.Execute(objectID);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mjkkirschner @QilongTang this is what we want instead of FitView. It focuses the view on the element, without changing the zoom. So we won't have the issue of causing users to lose a sense of where they are.

Only problem is, it appears to be broken...

It eventually calls this: https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs#L816-L848

As far as I can tell, this command isn't called anywhere in Dynamo. Which may explain its "only kind of working" state. It's kind of hard to explain what I mean by "only kind of working", but here it goes: currently, this command will focus the view on the element. Great. However, once the user goes to perform any other change to the view (pan, zoom in, zoom out, findByIdCommand, etc.), the view "forgets" it has been refocused on the element, and jumps back to its state before CenterViewOnElement was called. The behavior is even stranger when you use the findByIdCommand several times in a row. I'm not exactly sure what this problem is. Continuing to investigate. Considering making a fix to this problem as part of this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed: 2607752

It was updating the view but not the model.

@scottmitchell scottmitchell changed the title [WIP] View Extension API for ViewModel Delegate Commands View Extension API for ViewModel Delegate Commands Sep 1, 2019
@scottmitchell
Copy link
Collaborator Author

@mjkkirschner So far, I've only added FitView, FindById, and ForceRunExpression commands to the new command executive. I only added what was needed in the near term for TuneUp (actually, FitView is no longer needed but I've left it in for now). It sounds like we need to have a bigger discussion about what commands we want to add here. Assuming we want to add other commands, we can either add those in a later PR, or wait to merge this PR until we've added them here. I would prefer to add them to a later PR, so that TuneUp can move forward now. What do you think?

@mjkkirschner
Copy link
Member

@scottmitchell - makes sense to me, and thank for fixing this bug, I think it has caused jumpy behavior for a long time.

Are you able to kick off a build of master and do a push to Nuget? That way you. can consume the latest beta nugets that include your new apis. - I can do this if you do not have access.

It would also be good to run the self serve to see if any tests need to be updated for this change.

@scottmitchell
Copy link
Collaborator Author

@mjkkirschner I'm able to log into Jenkins and view history, but it looks like I no longer have permission to build, either self-serve or master. Is it possible to regain permissions? If not, do you mind starting the self serve for me? Will the nuget push happen automatically when the PR is merged?

@mjkkirschner
Copy link
Member

mjkkirschner commented Sep 1, 2019

@scottmitchell no, nuget push has to be enabled.
I can kick off self serve for this branch, but we'll need to do another build with nuget upload after this is merged.

@scottmitchell
Copy link
Collaborator Author

@mjkkirschner Self serve looks good

@mjkkirschner
Copy link
Member

LGTM

@mjkkirschner mjkkirschner added the LGTM Looks good to me label Sep 2, 2019
@scottmitchell scottmitchell merged commit 3fc290e into DynamoDS:master Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants