Removed dependencies in certain document-related classes

[Lehonti]

Now the removed dependencies are passed as arguments to methods.

This comes at the cost of introducing references to PintaCore in several places, which does not re-introduce them where they were before (which would trash our previous work): the dependencies are being obtained and coordinated at a more 'outer' layer of the code.

Notice that some of the places where the PintaCore references were introduced are tools (as in TextTool, PencilTool, ...). I can envision that being refactored so that the tools use services instead of PintaCore references.

The original goal was removing all dependencies (other than Document) from DocumentWorkspace, but it got quite large.

[cameronwhite]

There are likely going to be some merge conflicts from the DocumentWorkspace changes in #2046 so I'll wait to review this PR until that one lands

[Lehonti]

@cameronwhite since the pull request you mentioned has been merged, I think we can merge this one.

This is an intermediate step towards a specific goal. First, removing any dependencies on services from the constructor of Document and related classes, and after that, updating the code for the Farbfeld file format to reflect the changes, and finally developing the plug-in

[cameronwhite]

Thanks, will review later this week!
(BTW, the GitHub UI is still reporting that there are some merge conflicts)

[Lehonti]

@cameronwhite thank you.

And about the reported conflicts, I don't know why that might be. I am using the web UI and it doesn't report conflicts, which is why I left the changes as they are. Perhaps the pull request hasn't been refreshed locally?

[cameronwhite]

Ah, it had selected the "Rebase and merge" option - the squash mode seems fine so I think it's all good

[cameronwhite]

I don't see this tools parameter being used anywhere in the method?

[Lehonti]

You are right. I am not sure what the reason was for me to do this, and I can't test Pinta locally at this time, but after removing the parameter, the GitHub build actions succeed, so I think it's okay now

[cameronwhite]

Thanks, yeah it seems fine - the unused tools parameter might have been needed before #2046 or something like that

[cameronwhite]

I think the future fix here would be to avoid the dependency entirely - if the EditActions instead listened for history events (similar to the history pad) then it could update the disabled state of the undo/redo actions without needing the Document to be aware of this

If that seems easy to get working, then maybe we just do that instead of introducing and then removing these parameters? Otherwise I'm okay with the changes as an intermediate step

[Lehonti]

Yeah, that could very well be the case, and sorry if the refactoring seems a bit all over the place sometimes, but sometimes things that seem simple, like removing a dependency, end up touching a lot of files, and I have to 'slice' the planned changes to keep the difficulty of the review reasonable (like here...I planned to remove all dependencies from Document but I couldn't even get all of them out of DocumentWorkspace). In this case, Visual Studio reports that PushNewItem has 54 references. I would rather keep the refactoring you are suggesting for a future pull request, and frankly (at a first glance, looking at the call sites) I think it's quite a few intermediate steps away.

[cameronwhite]

Thinking about this more, I am a bit concerned with this intermediate step because it changes a commonly-used API for add-ins (for example, the PintaDemoExtension add-in has a BaseTool subclass which creates history items)
So if we don't end up being able to remove this new parameter before the next release, we'll have some unnecessary churn on bumping the add-in API version multiple times

Just to expand more on my idea from the previous comment: EditAcitons already subscribes to the "active document changed" event to update the state of the undo / redo buttons:

Pinta/Pinta.Core/Actions/EditActions.cs

Line 617 in 5cbe79c

private void WorkspaceActiveDocumentChanged (object? sender, EventArgs e)

)
So if this went a bit further and also listened for changes to the document's history stack (like the history panel does:

Pinta/Pinta.Gui.Widgets/Widgets/History/HistoryListView.cs

Line 98 in 5cbe79c

private void OnActiveDocumentChanged (object? sender, EventArgs e)

), then we don't need to pass EditActions into DocumentHistory at all

[Lehonti]

@cameronwhite I understand that concern and that you want to avoid breaking the API. Just for information, I am going through a very busy period, and I don't expect to be able to look into this (which is not just about the code changes, but also troubleshooting my local dev setup) until around the ~15th of May. There is a slim chance that I am able to take a look in the next four days, but even then it would be tough. Or if you wish you can merge this and take care of the remaining part.

[cameronwhite]

No problem! I might look at making the changes I suggested if I find some time