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

Use RelativeOffset instead of Offset #2401

Merged
merged 5 commits into from Jan 10, 2024

Conversation

charlenni
Copy link
Member

There is a RelativeOffset that derives from Offset and could be used better instead of the IsRelative flag of Offset. So I added a CalcOffset() function, that returns in Offset the offset and in RelativeOffset the new calculated offset from width and height and use this in all renderer calculations for the offset. So the code is easier and better readable.

Copy link
Member

@pauldendulk pauldendulk left a comment

Choose a reason for hiding this comment

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

Clearly a good improvement. This is the kind of thing to fix in this phase. I added one comment. Ideally you would want to abstract the implementation away. Perhaps this can be improved. If not I will merge it in this form.

One other thing. The CalcOffset method is now optional. In a new renderer the developer could forget to call it. It would be better if there is no way around it. I did not think of a good solution for that. I can think of ways to enforce it, but not nice ways.

@charlenni
Copy link
Member Author

Sorry @pauldendulk, but I couldn't see any comment.

Regarding the abstraction: I assume, that you talk about the CalcOffset() function in Offset. If yes, then this isn't possible. If we do this, then we have no implementation in Offset and have to use if's everywhere. More code.

Regarding that it could be forgotten: Yes, could be. But I couldn't think of any good strategy to force the developer to use it.

@pauldendulk
Copy link
Member

oh, I indeed, my comment is missing. I was talking about the fact that `is RelativeOffset' is still necessary in the renderer. A further improvement would be to abstract that away entirely behind some interface. That will probably be possible in some form or other (but not always better). It may need more basic changes. This is not something that you should look into.

I am striving for smaller PRs so I will merge this in it's current form. It is correct and an improvement. There are still some questions I have which could be investigated later. I will look into those myself.

Possible follow ups

  • Investigate if we can use just an interface in the renderer
  • There are (before and after this PR) two Offsets, one absolute and one relative. The relative one is now derived from the absolute one, which is a bit skewed. Better if there was an AbsoluteOffset derived from a base class.

@pauldendulk pauldendulk merged commit 5ff6649 into Mapsui:main Jan 10, 2024
5 checks passed
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.

None yet

2 participants