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

Integrated mytoolkit custom frame within mvvmcross #835

Closed
wants to merge 2 commits into
base: 3.2
from

Conversation

Projects
None yet
4 participants
@promontis
Contributor

promontis commented Nov 20, 2014

No description provided.

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Nov 20, 2014

Member

What is another implementation of INPC doing there? Also why all the ICommand implementations, did you just copy paste all of the code from mytoolkit?

Member

Cheesebaron commented Nov 20, 2014

What is another implementation of INPC doing there? Also why all the ICommand implementations, did you just copy paste all of the code from mytoolkit?

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Nov 20, 2014

Contributor

@Cheesebaron Jup... only made the mytoolkit implementation work with MvvmCross. I agree it needs to be cleaned up a bit by removing some duplicate logic. Could you help me out with that? What is your view on this PR?

ICommand implementation will be cleaned up to use MvxCommand (see other PR with discussion with @slodge).

What do mean by another implementation of INPC? Do you mean those classes like MvxWindowsLoadStateEventArg?

Contributor

promontis commented Nov 20, 2014

@Cheesebaron Jup... only made the mytoolkit implementation work with MvvmCross. I agree it needs to be cleaned up a bit by removing some duplicate logic. Could you help me out with that? What is your view on this PR?

ICommand implementation will be cleaned up to use MvxCommand (see other PR with discussion with @slodge).

What do mean by another implementation of INPC? Do you mean those classes like MvxWindowsLoadStateEventArg?

@Cheesebaron

This comment has been minimized.

Show comment
Hide comment
@Cheesebaron

Cheesebaron Nov 20, 2014

Member

What is the reasoning behind pulling in all this?

Member

Cheesebaron commented Nov 20, 2014

What is the reasoning behind pulling in all this?

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Nov 20, 2014

Contributor

@Cheesebaron Reason behind this pull... http://caliburnmicro.com/announcements/application-state-part-3/

This pull is the implementation of the Custom Frame

Contributor

promontis commented Nov 20, 2014

@Cheesebaron Reason behind this pull... http://caliburnmicro.com/announcements/application-state-part-3/

This pull is the implementation of the Custom Frame

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Nov 20, 2014

Contributor

@Cheesebaron ObservableObject will be removed in favor of MvxCommand

Contributor

promontis commented Nov 20, 2014

@Cheesebaron ObservableObject will be removed in favor of MvxCommand

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Nov 20, 2014

Contributor

@slodge @Cheesebaron have removed RelayCommand (and with that ObservableObject) in favor of MvxCommand. Using lazy instantiation as @slodge suggested

Contributor

promontis commented Nov 20, 2014

@slodge @Cheesebaron have removed RelayCommand (and with that ObservableObject) in favor of MvxCommand. Using lazy instantiation as @slodge suggested

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Nov 21, 2014

Contributor

This implementation also introduces one xaml file to the project. This will result in a xbf file and a pri file. It seems both are needed in a possible future nuget package. See also: http://www.bartlannoeye.com/blog/update-your-nuspec-file-to-include-xr-xml-files-in-your-nuget-package

Contributor

promontis commented Nov 21, 2014

This implementation also introduces one xaml file to the project. This will result in a xbf file and a pri file. It seems both are needed in a possible future nuget package. See also: http://www.bartlannoeye.com/blog/update-your-nuspec-file-to-include-xr-xml-files-in-your-nuget-package

@dbeattie71

This comment has been minimized.

Show comment
Hide comment
@dbeattie71

dbeattie71 Nov 22, 2014

Contributor

I looked at MyToolkit and it seems like it would be a pain to integrate and possibly not necessary. I added MyToolkit to my project via nuget and then because of the awesomeness of Mvx, implemented stuff to make it work. I created a new setup from MvxSetup, view dispatcher, and windows view presenter, and used the suspension manager from MyToolkit. I also made a change found here: http://goo.gl/AgualJ

I'll throw a sample together and put it on github.

Contributor

dbeattie71 commented Nov 22, 2014

I looked at MyToolkit and it seems like it would be a pain to integrate and possibly not necessary. I added MyToolkit to my project via nuget and then because of the awesomeness of Mvx, implemented stuff to make it work. I created a new setup from MvxSetup, view dispatcher, and windows view presenter, and used the suspension manager from MyToolkit. I also made a change found here: http://goo.gl/AgualJ

I'll throw a sample together and put it on github.

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Nov 22, 2014

Contributor

I like this - suspect it is what I'd personally want to use is I were writing a Win8.1 Jupiter app - I definitely want to see if we can merge this in somehow.

I'm wondering if the best way to do this is to put the MyToolkit integration in a separate assembly and nuget package.

This would mean that users could choose to stay with "nothing", could choose implement their own system, or could choose to use the MyToolkit integration. If we were really clever about this it might even be possible to pull MyToolkit in using nuget.

Depending on how we package and ship this, it could also then give people the freedom to develop and iterate independently of Mvx (and independent of me being slooooow)

e.g. we could take all of:

  • MvxWindowsFrame
  • MvxWindowsPage
  • MvxWindowsPageDescription
  • MvxWindowsSaveStateEventArgs
  • MvxWindowsLoadStateEventArgs
  • MvxWindowsNavigatingCancelEventArgs
  • IPageAnimation
  • TurnstilePageAnimation
  • NavigationKeyHandler
  • etc

One complicated point is what to do with MvxWindowsViewDispatcher and MvxWindowsViewPresenter which currently depend on RootFrame but in the changed code depend on MvxWindowsFrame. Looking at this it seems like there are three things the classes use from the frame:

  1. a rootFrame.Dispatcher property
  2. a set of Navigate, CanGoBack, Content and GoBack (or GoBackAsync) methods and properties (only Navigate is in the MS INavigate interface?)
  3. access to SetNavigationState and GetNavigationState

I think we could work out a way to hide these things behind interfaces - MvxWindowsFrame can then implement that interface and we can write an Adapter or shim to wrap Frame.

We could then change MvxWindowsSetup, MvxWindowsViewDispatcher, MvxWindowsViewPresenter to use these interfaces and users can then choose whether they plumb in vanillaWinRT, MyToolkit, or something else....

Hope that doesn't all sound too insane... I'll go do some prototyping to see if I can explain better in code...

Contributor

slodge commented Nov 22, 2014

I like this - suspect it is what I'd personally want to use is I were writing a Win8.1 Jupiter app - I definitely want to see if we can merge this in somehow.

I'm wondering if the best way to do this is to put the MyToolkit integration in a separate assembly and nuget package.

This would mean that users could choose to stay with "nothing", could choose implement their own system, or could choose to use the MyToolkit integration. If we were really clever about this it might even be possible to pull MyToolkit in using nuget.

Depending on how we package and ship this, it could also then give people the freedom to develop and iterate independently of Mvx (and independent of me being slooooow)

e.g. we could take all of:

  • MvxWindowsFrame
  • MvxWindowsPage
  • MvxWindowsPageDescription
  • MvxWindowsSaveStateEventArgs
  • MvxWindowsLoadStateEventArgs
  • MvxWindowsNavigatingCancelEventArgs
  • IPageAnimation
  • TurnstilePageAnimation
  • NavigationKeyHandler
  • etc

One complicated point is what to do with MvxWindowsViewDispatcher and MvxWindowsViewPresenter which currently depend on RootFrame but in the changed code depend on MvxWindowsFrame. Looking at this it seems like there are three things the classes use from the frame:

  1. a rootFrame.Dispatcher property
  2. a set of Navigate, CanGoBack, Content and GoBack (or GoBackAsync) methods and properties (only Navigate is in the MS INavigate interface?)
  3. access to SetNavigationState and GetNavigationState

I think we could work out a way to hide these things behind interfaces - MvxWindowsFrame can then implement that interface and we can write an Adapter or shim to wrap Frame.

We could then change MvxWindowsSetup, MvxWindowsViewDispatcher, MvxWindowsViewPresenter to use these interfaces and users can then choose whether they plumb in vanillaWinRT, MyToolkit, or something else....

Hope that doesn't all sound too insane... I'll go do some prototyping to see if I can explain better in code...

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Nov 22, 2014

Contributor

So... I had a play - see 0652e11 - obviously this isn't a complete cleanup, just an idea I'm sharing.

I think by introducing an IMvxWindowsFrame level of interface, we might be able to allow any Frame to be adapted for use within Mvx - including the legacy Frame and the fab MyToolkit frame.

public interface IMvxWindowsFrame
{
    Control UnderlyingControl { get; }
    CoreDispatcher Dispatcher { get; }
    object Content { get; }
    bool CanGoBack { get; }
    bool Navigate(Type viewType, object parameter);
    void GoBack();
    void ClearValue(DependencyProperty property);
    object GetValue(DependencyProperty property);
    void SetValue(DependencyProperty property, object value);
    void SetNavigationState(string state);
    string GetNavigationState();
}



public class MvxFrameAdapter : IMvxWindowsFrame
{
    private readonly Frame _frame;

    public MvxFrameAdapter(Frame frame)
    {
        _frame = frame;
    }

    public Control UnderlyingControl { get { return _frame; } }

    public CoreDispatcher Dispatcher { get { return _frame.Dispatcher; } }
    public object Content { get { return _frame.Content; } }
    public bool CanGoBack { get { return _frame.CanGoBack; } }

    public bool Navigate(Type viewType, object parameter)
    {
        return _frame.Navigate(viewType, parameter);
    }

    public void GoBack()
    {
        _frame.GoBack();
    }

    public void ClearValue(DependencyProperty property)
    {
        _frame.ClearValue(property);
    }

    public object GetValue(DependencyProperty property)
    {
        return _frame.GetValue(property);
    }

    public void SetValue(DependencyProperty property, object value)
    {
        _frame.SetValue(property, value);
    }

    public void SetNavigationState(string state)
    {
        _frame.SetNavigationState(state);
    }

    public string GetNavigationState()
    {
        return _frame.GetNavigationState();
    }
}

If we went this route then I think for MyToolkit we could:

  • write a new assembly (whether it's in Mvx Repo or somewhere else is debateable),
  • consume MyToolkit as a nuget package within this,
  • then just write a small Adapter class to marshall between IMvxWindowsFrame and the MyToolkit Frame

I think the advantages in doing that would be:

  • other users could use other Frames
  • no one would need to duplicate the MyToolkit codebase - so future maintenance should be easier... (we can dream :))

(The only disadvantage I can see is that there would be a possibly overlap between things like RelayCommand and MvxCommand ... only way around that I can think of is if MyToolkit could somehow be split at a binary level... but I don't think this would be a showstopper issue...)

What do you think....

Contributor

slodge commented Nov 22, 2014

So... I had a play - see 0652e11 - obviously this isn't a complete cleanup, just an idea I'm sharing.

I think by introducing an IMvxWindowsFrame level of interface, we might be able to allow any Frame to be adapted for use within Mvx - including the legacy Frame and the fab MyToolkit frame.

public interface IMvxWindowsFrame
{
    Control UnderlyingControl { get; }
    CoreDispatcher Dispatcher { get; }
    object Content { get; }
    bool CanGoBack { get; }
    bool Navigate(Type viewType, object parameter);
    void GoBack();
    void ClearValue(DependencyProperty property);
    object GetValue(DependencyProperty property);
    void SetValue(DependencyProperty property, object value);
    void SetNavigationState(string state);
    string GetNavigationState();
}



public class MvxFrameAdapter : IMvxWindowsFrame
{
    private readonly Frame _frame;

    public MvxFrameAdapter(Frame frame)
    {
        _frame = frame;
    }

    public Control UnderlyingControl { get { return _frame; } }

    public CoreDispatcher Dispatcher { get { return _frame.Dispatcher; } }
    public object Content { get { return _frame.Content; } }
    public bool CanGoBack { get { return _frame.CanGoBack; } }

    public bool Navigate(Type viewType, object parameter)
    {
        return _frame.Navigate(viewType, parameter);
    }

    public void GoBack()
    {
        _frame.GoBack();
    }

    public void ClearValue(DependencyProperty property)
    {
        _frame.ClearValue(property);
    }

    public object GetValue(DependencyProperty property)
    {
        return _frame.GetValue(property);
    }

    public void SetValue(DependencyProperty property, object value)
    {
        _frame.SetValue(property, value);
    }

    public void SetNavigationState(string state)
    {
        _frame.SetNavigationState(state);
    }

    public string GetNavigationState()
    {
        return _frame.GetNavigationState();
    }
}

If we went this route then I think for MyToolkit we could:

  • write a new assembly (whether it's in Mvx Repo or somewhere else is debateable),
  • consume MyToolkit as a nuget package within this,
  • then just write a small Adapter class to marshall between IMvxWindowsFrame and the MyToolkit Frame

I think the advantages in doing that would be:

  • other users could use other Frames
  • no one would need to duplicate the MyToolkit codebase - so future maintenance should be easier... (we can dream :))

(The only disadvantage I can see is that there would be a possibly overlap between things like RelayCommand and MvxCommand ... only way around that I can think of is if MyToolkit could somehow be split at a binary level... but I don't think this would be a showstopper issue...)

What do you think....

@dbeattie71

This comment has been minimized.

Show comment
Hide comment
@dbeattie71

dbeattie71 Nov 23, 2014

Contributor

I see what you were referring to now, looks good.

Contributor

dbeattie71 commented Nov 23, 2014

I see what you were referring to now, looks good.

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Nov 23, 2014

Contributor

I really like the interface you introduced! Opens MvvmCross up to allow more flexible use of the Frame. The PR I provided was just to test if the code would actually work and start up the discussion. Great to see that we were able to extract this into an interface!

I don't think it is wise to depend on the MyToolkit from any of the MvvmCross NuGet packages. I do think we should include the plain vanilla Frame adapter in MvvmCross. Any custom Frame implementation should be put in a separate packages.

From my pont of view, the disadvantage of the overlap between RelayCommand and MvxCommand could be resolved by re-coding the MyToolkit classes to use MvxCommand (like I did in this PR). Don't know if the author of MyToolkit approves as we are copying his code but that would basically solve the issue (and shrink the user IL code size)

Let me try to pull in your commit and use the interface in the current implementation. I'll extract my implementation in a separate library. I'll also try to create the MyToolkit adapter.

Contributor

promontis commented Nov 23, 2014

I really like the interface you introduced! Opens MvvmCross up to allow more flexible use of the Frame. The PR I provided was just to test if the code would actually work and start up the discussion. Great to see that we were able to extract this into an interface!

I don't think it is wise to depend on the MyToolkit from any of the MvvmCross NuGet packages. I do think we should include the plain vanilla Frame adapter in MvvmCross. Any custom Frame implementation should be put in a separate packages.

From my pont of view, the disadvantage of the overlap between RelayCommand and MvxCommand could be resolved by re-coding the MyToolkit classes to use MvxCommand (like I did in this PR). Don't know if the author of MyToolkit approves as we are copying his code but that would basically solve the issue (and shrink the user IL code size)

Let me try to pull in your commit and use the interface in the current implementation. I'll extract my implementation in a separate library. I'll also try to create the MyToolkit adapter.

slodge pushed a commit that referenced this pull request Nov 23, 2014

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Nov 23, 2014

Contributor

Thanks both - really happy to refactor the interfaces to make them more usable - and I think the breaking change shouldn't be significant for most existing users.

Have pushed the changes to the head of 3.5

Personally, I'd love to see your MyToolkit-based Frame in a new repo and in a new nuget package - I think it should be possible for users to pull it in instead of pulling in the MvvmCross starter pack (just for those WindowsCommon UI projects). Really like what you've done and glad this interface works for all of us :)

Contributor

slodge commented Nov 23, 2014

Thanks both - really happy to refactor the interfaces to make them more usable - and I think the breaking change shouldn't be significant for most existing users.

Have pushed the changes to the head of 3.5

Personally, I'd love to see your MyToolkit-based Frame in a new repo and in a new nuget package - I think it should be possible for users to pull it in instead of pulling in the MvvmCross starter pack (just for those WindowsCommon UI projects). Really like what you've done and glad this interface works for all of us :)

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Dec 11, 2014

Contributor

@slodge it seems that for this forked implementation this line actually creates the View, not injecting anything. Currently working with this fork in a project of mine to straighten out some bugs, and while trying to inject a IMvxMessenger in one of my views, I got an MissingMethodException which led me to this method.

Could you point me to the MvvmCross implementation of creating Views, and thus optionally injecting registered dependencies?

@slodge it seems that for this forked implementation this line actually creates the View, not injecting anything. Currently working with this fork in a project of mine to straighten out some bugs, and while trying to inject a IMvxMessenger in one of my views, I got an MissingMethodException which led me to this method.

Could you point me to the MvvmCross implementation of creating Views, and thus optionally injecting registered dependencies?

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Dec 12, 2014

Contributor

Could you point me to the MvvmCross implementation of creating Views, and thus optionally injecting registered dependencies?

@promontis Not sure what you are asking. WinRT creates Pages/Controls, not Mvx?

Contributor

slodge commented Dec 12, 2014

Could you point me to the MvvmCross implementation of creating Views, and thus optionally injecting registered dependencies?

@promontis Not sure what you are asking. WinRT creates Pages/Controls, not Mvx?

@promontis

This comment has been minimized.

Show comment
Hide comment
@promontis

promontis Dec 12, 2014

Contributor

@slodge Nevermind... I traced things back to MvxWindowsViewPresenter, which calls _rootFrame.Navigate(viewType, request); So Mvx is delegating the creation of views to the root frame. This custom implementation uses the custom root frame, which is not able to actually inject any dependencies into views. I'll look into how a normal WinRT frame creates the view and injects dependencies

Contributor

promontis commented Dec 12, 2014

@slodge Nevermind... I traced things back to MvxWindowsViewPresenter, which calls _rootFrame.Navigate(viewType, request); So Mvx is delegating the creation of views to the root frame. This custom implementation uses the custom root frame, which is not able to actually inject any dependencies into views. I'll look into how a normal WinRT frame creates the view and injects dependencies

@slodge

This comment has been minimized.

Show comment
Hide comment
@slodge

slodge Feb 8, 2015

Contributor

Thanks - I'm going to close this one now without merging - as I think/hope the interface approach gives us a chance to replace the RootFrame.

If more is needed here - e.g. a refactoring of the interface then happy to do it - we should be able to do this without breaking anyone's apps - this is a new interface :)

Contributor

slodge commented Feb 8, 2015

Thanks - I'm going to close this one now without merging - as I think/hope the interface approach gives us a chance to replace the RootFrame.

If more is needed here - e.g. a refactoring of the interface then happy to do it - we should be able to do this without breaking anyone's apps - this is a new interface :)

@slodge slodge closed this Feb 8, 2015

@promontis promontis deleted the promontis:3.2 branch May 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment