Skip to content

Removed dependencies in certain document-related classes#2073

Open
Lehonti wants to merge 11 commits intoPintaProject:masterfrom
Lehonti:improvement1
Open

Removed dependencies in certain document-related classes#2073
Lehonti wants to merge 11 commits intoPintaProject:masterfrom
Lehonti:improvement1

Conversation

@Lehonti
Copy link
Copy Markdown
Contributor

@Lehonti Lehonti commented Mar 24, 2026

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor Author

Lehonti commented Apr 20, 2026

@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
Copy link
Copy Markdown
Member

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

@Lehonti
Copy link
Copy Markdown
Contributor Author

Lehonti commented Apr 21, 2026

@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
Copy link
Copy Markdown
Member

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

Comment thread Pinta.Core/Classes/DocumentWorkspace.cs Outdated
public double Scale
=> ViewSize.Width / (double) document.ImageSize.Width;

public void SetScale (double value, ToolManager tools)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

public IEnumerable<BaseHistoryItem> Items => history;

public void PushNewItem (BaseHistoryItem newItem)
public void PushNewItem (BaseHistoryItem newItem, EditActions edit)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@Lehonti Lehonti Apr 25, 2026

Choose a reason for hiding this comment

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

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.

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