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

Fix Printer class member 'content_offset' naming #672

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Apitronix
Copy link
Contributor

Replace "content_offset" by "drawing_area_offset".

@gyscos
Copy link
Owner

gyscos commented Aug 23, 2022

Hi, and thanks for the work!

I'm not sure this renaming is necessary. In addition to the breaking change it would cause, it may not be much clearer.

First, let's summarize the various values. One of the big concepts is "real size" vs "virtual size", and it mostly matters when scrolling (or cropping) is involved.
A view can think it's given a large size, but only a subset of that will actually be visible at a given time. Most of the time, the view doesn't have to care what part is visible. It should mostly be used for optimizations (if it wants to avoid spending time on parts that will not be visible anyway).

Let's define a few values:

  • In the view coordinates, the "virtual area" allocated to the view is Rect::from_size((0,0), printer.size).
  • In the view coordinates, the subset that is actually visible in the end is Rect::from_size(printer.content_offset, printer.output_size)
  • In the terminal coordinates, the area that can be drawn on is Rect::from_size(printer.offset, printer.output_size)

From this, we can see that content_offset is the "offset into the content", used to only draw a subset of a view. If a view has a "size" of 10x10, but we only want to draw the bottom 2x2 corner (maybe because we are scrolling into this large view), the content offset will be 8x8. Calling it drawing_area_offset can be confusing as it's not really an offset into the drawing area.

For output_size, I agree it could be called drawing_area_size, since it's the size where actual drawing is allowed, but I'm not sure it justifies the breaking change. In addition, it could still be mis-interpreted as being the size given to draw a view.

I do agree that the naming are not ideal, but maybe what it needs is more examples, and/or viewport functions that return the Rect I defined earlier - these should help understand where each value fits in.

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