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.

Sign up for GitHub

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

Jump to bottom

Re-export druid_shell::Scalable & remove to_px/to_dp helpers on Scale #1075

Merged
merged 1 commit into from
Jul 14, 2020
Merged

Re-export druid_shell::Scalable & remove to_px/to_dp helpers on Scale #1075

merged 1 commit into from
Jul 14, 2020

Conversation

ForLoveOfCats
Copy link
Collaborator

@ForLoveOfCats ForLoveOfCats commented Jul 2, 2020 •
edited
Loading

This allows use-ing Scalable and thus various type's implementation of to_px ect

@ForLoveOfCats ForLoveOfCats added the S-needs-review waits for review label Jul 2, 2020
@cmyr
Copy link
Member

cmyr commented Jul 4, 2020

@xStrom does this make sense as public API to you?

@ForLoveOfCats
Copy link
Collaborator Author

ForLoveOfCats commented Jul 4, 2020 •
edited
Loading

It kinda already is. Scale has the public and very confusingly named to_px which accepts a generic arg which impls Scalable before calling the argument's own to_px passing in the instance of Scale (Which does not work in application code due to being unable to bring Scalable into scope). I say it is confusingly named because for the types which implement Scalable their to_px functions convert themselves using the Scale passed in but this apparent helper function on Scale does the opposite by converting the argument using itself as the scale.

When I initially drafted this PR I hadn't looked into these functions to much but now after using this branch for my project and poking around I think that the helper function on Scale should be removed and simply use the methods on these Scalable types. (Which I'll do and force push to this PR if others agree) The alternative could be to rename the helper function but I'd argue that it has no reason to exist as it is a single line to invoke a different function which is part of a trait anyway

My specific use case of having this functionality provided to applications is that I'm using it to handle font size in a DPI aware way by converting to absolute pixel values.

@ForLoveOfCats
Copy link
Collaborator Author

I've removed the helper functions and corrected any callsites I could find with a quick grep. I decided to leave the px_to_dp* functions as despite not finding any places which call any of them they seem reasonable enough to exist on Scale as is. I expect the CI to probably fail on some non-GTK shell

@ForLoveOfCats ForLoveOfCats force-pushed the ReExportScalable branch 4 times, most recently from ddd8ec1 to 3dccf7c Compare July 8, 2020 19:42
@ForLoveOfCats ForLoveOfCats changed the title Re-export druid_shell::Scalable Re-export druid_shell::Scalable & remove to_px/to_dp helpers on Scale Jul 8, 2020
@ForLoveOfCats
Copy link
Collaborator Author

After fixing the CI and fiddling with merge conflicts I think this is ready for a review. Thankfully removing those functions turned out to be a fairly small change

@ForLoveOfCats ForLoveOfCats added the breaking change pull request breaks user code label Jul 8, 2020
luleyleo
luleyleo approved these changes Jul 12, 2020
View reviewed changes
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

This seems reasonable to me. Just one nit about the changlog.

CHANGELOG.md Outdated Show resolved Hide resolved
@luleyleo luleyleo removed the S-needs-review waits for review label Jul 12, 2020
@ForLoveOfCats ForLoveOfCats force-pushed the ReExportScalable branch 4 times, most recently from ef0a7f0 to b6f0cf9 Compare July 12, 2020 17:34
@ForLoveOfCats ForLoveOfCats reopened this Jul 12, 2020
@ForLoveOfCats
Copy link
Collaborator Author

So none of the CI seems to be running and the changelog still has a merge conflict after manually fixing it so I'm just going to let this sit for a while and hope Github sorts itself out

luleyleo
luleyleo reviewed Jul 12, 2020
View reviewed changes
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

I can't spot the changes made in #1081 so maybe those are missing?

CHANGELOG.md Outdated

### Deprecated

### Removed

- `Scale::from_dpi`, `Scale::dpi_x`, and `Scale::dpi_y`. ([#1042] by [@xStrom])
- `Scale::to_px` and `Scale::to_dp` ([#1075] by [@ForLoveOfCats])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- `Scale::to_px` and `Scale::to_dp` ([#1075] by [@ForLoveOfCats])
- `Scale::to_px` and `Scale::to_dp`. ([#1075] by [@ForLoveOfCats])

CHANGELOG.md Outdated
@@ -15,12 +15,14 @@ You can find its changes [documented below](#060---2020-06-01).
- `Scale::from_scale` to `Scale::new`, and `Scale` methods `scale_x` / `scale_y` to `x` / `y`. ([#1042] by [@xStrom])
- Major rework of keyboard event handling ([#1049] by [@raphlinus])
- `Container::rounded` takes `KeyOrValue<f64>` instead of `f64`. ([#1054] by [@binomial0])
- Re-export `druid_shell::Scalable` under `druid` namespace ([#1075] by [@ForLoveOfCats])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Re-export `druid_shell::Scalable` under `druid` namespace ([#1075] by [@ForLoveOfCats])
- Re-export `druid_shell::Scalable` under `druid` namespace. ([#1075] by [@ForLoveOfCats])

@ForLoveOfCats
Copy link
Collaborator Author

GH claims there is only one conflict and it is the following
image
Combined with the CI bugging out I think that Github is being silly so I'm going to let this sit for a bit and see if it figures itself out

@ForLoveOfCats ForLoveOfCats force-pushed the ReExportScalable branch 2 times, most recently from b6f0cf9 to 70d2ec1 Compare July 13, 2020 18:46
@ForLoveOfCats
Copy link
Collaborator Author

I managed to fix the merge conflict by just moving the conflicting line up a line, not ideal but better than screwing up the PR's git log. The CI also seems to be functioning again, not sure if the conflict was blocking the CI from running as I'm 90% sure that I've had PRs run CI with merge conflicts but I don't know if the CI runs on just the PR's branch or on the master branch w/ the PR merged which would cause it to be blocked 🤷

@ForLoveOfCats ForLoveOfCats merged commit 5ec6ca4 into linebender:master Jul 14, 2020
@xStrom
Copy link
Member

xStrom commented Jul 16, 2020

Sorry for not taking a look sooner, but yeah I think this change is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luleyleo luleyleo luleyleo approved these changes

Assignees
No one assigned
Labels
breaking change pull request breaks user code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ForLoveOfCats @cmyr @xStrom @luleyleo