Issues with MVVM Implementation

[StefanKoell]

Hi,

I'm trying to wrap my head around the MVVM approach and found an issue which prevents me to use a ViewLocator (IDataTemplate interface).

Setting the ContentTemplateResourceKey does not work when:

• the resource is defined in the App.xaml or any other Resource dictionary in the control hierarchy up. Expected behavior would be to search all merged resource dictionaries to find the template.
• the resource is an IDataTemplate (view locator), it doesn't work as it expects an Avalonia Control instead. I guess the Method here https://github.com/npolyak/NP.Ava.UniDock/blob/ab64223943108199148d62f094fa9e4688d1eb3c/src/IDockGroup.cs#L362 should also handle IDataTemplate

Regards, Stefan

[npolyak]

Can you publish in a public repository on Github a sample you want to work and I'll try to make it work on Sunday?

I do not quite understand what you want to do here.

Regards,

Nick

[StefanKoell]

I just submitted 2 PRs which changes the code in a way that it works with DataTemplates and IDataTemplate as well. The original DataContextSample still works (haven't tested all the other solutions though).

Btw., may I ask why you chose to split all the packages in multiple repos with submodules? Having a single repo with multiple projects/solutions/packages would be much more community friendly in terms of contributing PRs and building from source. I'm very open to help improving the codebase and a simpler solution/project structure would be very helpful.

[StefanKoell]

The other two (minor) issues:

• resource lookup up the tree to the app.xaml merged dictionary
• make the DockItemsViewModels property bindable from XAML

Are not addressed yet and I could do separate PRs for that if you like.

[npolyak]

Btw., may I ask why you chose to split all the packages in multiple repos with submodules? Having a single repo with multiple projects/solutions/packages would be much more community friendly in terms of contributing PRs and building from source. I'm very open to help improving the codebase and a simpler solution/project structure would be very helpful.

Because submodules is the best way to actually split and share the functionality in git. Submodules is the best way to make people think about where the functionality actually belongs and figure out the project dependencies. NP.Ava.Visuals - can be used by other projects than NP.Ava.UniDock so there is no reason for them to be mixed together.

I've been working a lot with submodules - I'll be happy to help you with git commands if you need.

And I'll be happy if you participate, BTW - I have very limited time to work on it aside from my vacations.

[npolyak]

I just submitted 2 PRs which changes the code in a way that it works with DataTemplates and IDataTemplate as well. The original DataContextSample still works (haven't tested all the other solutions though).

I saw only 1 PR, btw

[StefanKoell]

Hi Nick,

you can see both PRs linked in this issue above:

The PR in this repo (UniDock) is merged already but the one in the Visuals repo is not yet: https://github.com/npolyak/NP.Ava.Visuals/pull/1

[StefanKoell]

Because submodules is the best way to actually split and share the functionality in git. Submodules is the best way to make people think about where the functionality actually belongs and figure out the project dependencies. NP.Ava.Visuals - can be used by other projects than NP.Ava.UniDock so there is no reason for them to be mixed together.

Well, I'm also using submodules and there are good use cases. The organization in git repos (using submodules) and the way packages are created doesn't necessary need to mirror the same structure. You can have one git repo containing one or more solutions with many projects all creating and pushing their own, dedicated nuget packages.

From the "consumer" point of view, it's great the way it is and should stay that way. The packages and their dependencies stays the same and everyone can pick and choose what to use.

From the "maintenance" point of view a single repo would make it easier to clone, change and submit PRs. Cloning a repo, opening a solution (preferably in the /src root), hit compile, test, create packages, test the implementation would be much easier.

Right now, I have to fork, clone, branch and submit PRs in every single repo to fully maintain the project. I then also need create the packages locally from different repos to test the implementation before submitting PRs. My change "only" affected 2 repositories, so I needed to submit two dedicated PRs after I forked the two repos. I can imagine that this also gets harder if more people are submitting PRs. Having a lot of different branches scattered in multiple repos will get overwhelming fast, I guess. The more repos are involved the more cumbersome the PRs become.

Anyway, it was only a suggestion and I don't really know all the reasons you chose this structure. I can live with that. If you reconsider to consolidate the submodules into a single repo (or fewer repos), I'm also happy to help.

Again, thanks for the great work and for merging my PR. It helps a lot with MVVM!

[npolyak]

Well, especially if there are many people work on a project (and I hope you and other people will chime it) from my point of view it is better to make it more difficult to modify the projects that other projects depend on. Even for myself - after taking several months long break the structure helps me to avoid some errors e.g. forgetting to create new package versions when a project was modified.

To make it plainer when several people are working on a code - it makes it more difficult to mess things up by making people thing about the dependencies, though also makes it more difficult to update projects.

You do not have to clone every SubModule - all you have to do is to use

git submodule update --init

and

git submodule update 

commands.

[npolyak]

I am looking at your other questions - as far as I remember DockItemsViewModel was used only for serialization, not for bindings, but by the end of my day - I'll tell you for sure if it should be made XAML bindable or not.

Thanks for your paticipation!

[StefanKoell]

Well, especially if there are many people work on a project (and I hope you and other people will chime it) from my point of view it is better to make it more difficult to modify the projects that other projects depend on. Even for myself - after taking several months long break the structure helps me to avoid some errors e.g. forgetting to create new package versions when a project was modified.

To make it plainer when several people are working on a code - it makes it more difficult to mess things up by making people thing about the dependencies, though also makes it more difficult to update projects.

You do not have to clone every SubModule - all you have to do is to use

git submodule update --init

and

git submodule update 

commands.

I guess you are not creating PRs yourself for the code changes, right? I can understand your argument working on the code base solo. Pushing changes direct to the repos (without PRs), you are right and you only need the submodules initialized and updated.

However, working with the PR workflow, this doesn't suffice. In order to create a PR on github, you first need to fork a repo, create a feature or bugfix branch, make the changes on the forked repo and push it into the forked repo first. Then you create a PR. So far so good.

The problem with the submodules is that all the submodules of the forked version are still pointing to the original github repos. Which means, I can't simply update code in those submodules because they are not the forked repos.

So right now, I have to change the code in the forked version but if I touched code in one of the submodules, I have to fork and clone these repos as well in order to create a clean PR.

I'm not sure if I explained the issue good enough as english is not first language but working clean PRs is very cumbersome in the current structure. Besides, PRs, code review and approval is actually the tool of choice to ensure that somebody doesn't mess up.

Practicle example: my two separate PRs in different repos actually led you to believe that there's only one PR and you actually didn't see the other PR from the other repo. The danger to mess something up in this structure is actually much higher. I haven't really tested if the changes I made worked with only one PR merged but chances are, that the incomplete approval of the PRs actually broke the packages.

Also, from a testing perspective let me mention that a single repo would make it quite easy to verify if a PR is breaking something or not because you only need to pull the PR's branch instead of all the involved github modules and make sure that the referenced submodules (which are not pointing to the correct branch) are not mistakenly used.

Take a look at the Avalonia repo itself. It's a huge repo but it's a single repo although multiple packages are created from it and the way PRs are verified and approved is actually working great to ensure high quality code contributions.

[StefanKoell]

One more thing: since I'm not sure I was able to explain myself well, I recommend you try the PR workflow yourself by creating a second github account, try to change something which affects more than 2 repos and try to make code changes on a clean machine which only works with the forked repos. If you do this exercise, you will see what I mean with the complicated workflow ;)

[npolyak]

I understand. You are correct I am not forking my own repos and I am not creating the PRs for myself.

I'll try tonight or likelier next Sunday and will see what can be done to improve the workflow.

Yes, I know Avalonia code - but there are several of people on Avalonia team and they are working on Avalonia full time while for me UniDock and the my other repositories are a side project - I need to work more than full time to provide for my family. So I am a bit more worried about the code becoming a mess.

[npolyak]

Instead of forking the repositories - can you create a branch, check it out and do the same for the submodule(s) you want to modify.

At least this is what I want the workflow to be for other people interested. So if it is not possible now, perhaps I can try to fix the permissions to make it possible.

[StefanKoell]

I strongly recommend to use the PR workflow instead of pushing changes directly to the repo. Pushing directly to the repo is far more dangerous (and not really considered best practice) than using the PR workflow.

As the owner of the repo you can still push directly (I do that too for my repos) but at the same time allow others to contribute in a clean and secure way. So to clarify: the "test" I recommended for you to try the PR workflow was not saying you should always use PRs for your own changes - just to try it out to see what contributors should/need to do to submit code.

Besides, you could, of course create PRs directly on your repo as an owner if you like.

[npolyak]

I am not talking about not doing the PRs. Of course the commits should be merged to the main branch via the PRs.

I am talking only about not using the forks. Instead of creating a fork, you clone the original repository, then create your own branch. Then commit your own branch and create a PR to the main branch.

This is how it worked in many of my contracts and I do not see why it should not work now.

In other words you still won't be able to commit your change straight to the main branches - you'll need PRs for that.

[StefanKoell]

This would require you to allow everyone to push to your repo. This may be a viable solution for contract work with a limited number of contributors but for open source repos it's quite uncommon and surely not best practice. Not sure if you really want to do this ;)

But yeah, if that's your choice, no problem. Still, considering that pushing code changes which also affects one or more submodules, needs careful consideration with consistent branch names (across all repos) and the danger (like in my case) that PRs only get partially approved because changes from other repos may be overlooked.

[npolyak]

We'll start with this strategy and if it does not work - we'll change it.

a repository can definitely handle 25 people pushing their work in it and the main branches will be clean because of my supervision of the PRs. If we see some downside to it in the future - perhaps we'll do smth more radical.

[StefanKoell]

Fair enough! I will close this one now because the PRs were merged. Will continue to work with the packages and may submit other PRs down the road. Thanks for the discussion, insights and your work. I really appreciate it!

[npolyak]

Thanks Stefan, I also appreciate you joining the effort!

[npolyak]

I added firing PropertyChanged event to some of the DockItemsViewModelBase. The properties should not change for the life duration of the object.

[npolyak]

@StefanKoell I added you as a collaborator to 5 of my repositories including the main one NP.Ava.UniDock. The main branches in each one is protected. So you do not have to use forks - you can check out the NP.Ava.UniDock repository, initialize the submodules, do changes in your own branches and create the pull requests.

[StefanKoell]

Accepted all the invitations. Thank you!