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

rename WidgetPod::paint_with_offset to just 'paint' #962

Closed
cmyr opened this issue May 19, 2020 · 6 comments · Fixed by #980
Closed

rename WidgetPod::paint_with_offset to just 'paint' #962

cmyr opened this issue May 19, 2020 · 6 comments · Fixed by #980
Assignees
Labels
breaking change pull request breaks user code D-Easy just needs to be implemented

Comments

@cmyr
Copy link
Member

cmyr commented May 19, 2020

This has been a long-standing todo, and I think it makes sense; it's a bit of a gotcha right now.

What is currently 'paint' can get some other more annoying name.

@cmyr cmyr added the breaking change pull request breaks user code label May 19, 2020
@xStrom
Copy link
Member

xStrom commented May 19, 2020

What's the benefit of the current paint? Just performance when the origin matches? I'm thinking how often does it even get used and for what purpose.

For now I'm thinking perhaps something like the following:

Current name New name
paint_with_offset paint
paint_with_offset_always paint_always
paint paint_with_current_origin

@cmyr
Copy link
Member Author

cmyr commented May 19, 2020

I think that paint might get called by the others, after applying the transform? I'm not 100% sure though. Those names look good, maybe paint_raw instead of paint_with_current_origin but just spitballing.

@luleyleo luleyleo added D-Easy just needs to be implemented help wanted has no one working on it yet labels May 21, 2020
@totsteps
Copy link
Collaborator

totsteps commented May 22, 2020

I will make a PR for this. As per discussion here's how I will rename things:

Current Name New name
paint paint_raw
paint_with_offset paint
paint_with_offset_always paint_always

Should we proceed with these renamings?

@totsteps totsteps self-assigned this May 22, 2020
@xStrom
Copy link
Member

xStrom commented May 22, 2020

Yeah I think those names are fine. Remember to also update documentation references where it makes sense.

@xStrom xStrom removed the help wanted has no one working on it yet label May 22, 2020
@totsteps
Copy link
Collaborator

Once the paint_with_offset is renamed to paint there will be name conflict at line druid/src/core.rs:372 and 373.

Not sure how to resolve this!?

@xStrom
Copy link
Member

xStrom commented May 22, 2020

- their [`paint`] method.
+ [`Widget::paint`].

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change pull request breaks user code D-Easy just needs to be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants