-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 MRProgressView compiling issue #21235
Conversation
Podfile
Outdated
# We are going to replace the `id` with `MRProgressView *` (or any other type that has a method `setProgress:` that takes a float argument). | ||
mrprogress_filepath = "#{project_root}/Pods/MRProgress/src/Components/MRProgressOverlayView.m" | ||
File.chmod(0o644, mrprogress_filepath) | ||
lines = IO.readlines(mrprogress_filepath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 | File.readlines is safer than IO.readlines . |
Podfile
Outdated
lines = IO.readlines(mrprogress_filepath) | ||
if lines[810] == " [((id)self.modeView) setProgress:self.progress];\n" | ||
lines[810] = " [((MRProgressView *)self.modeView) setProgress:self.progress];\n" | ||
IO.write(mrprogress_filepath, lines.join) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚫 | File.write is safer than IO.write . |
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
if lines[810] == " [((id)self.modeView) setProgress:self.progress];\n" | ||
lines[810] = " [((MRProgressView *)self.modeView) setProgress:self.progress];\n" | ||
File.write(mrprogress_filepath, lines.join) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nifty way of patching the issue 🤓
Have you considered forking the repo? Might set us up for addressing future issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That didn't cross my mind. 🥲
But I did think about removing this library, because it's only used in one single place: showing an activity indicator during uploading a featured image to a post's settings. I got lazy and decided to make this one line change instead. 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
I came here to write the same thing. I found that same as you did (sigh of relief).
If we go in with the intention of removing this library, then I'd say it's okay to merge this approach. I wonder if we could get the help of some iOS UI dev 🤓
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
if lines[810] == " [((id)self.modeView) setProgress:self.progress];\n" | ||
lines[810] = " [((MRProgressView *)self.modeView) setProgress:self.progress];\n" | ||
File.write(mrprogress_filepath, lines.join) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
I came here to write the same thing. I found that same as you did (sigh of relief).
If we go in with the intention of removing this library, then I'd say it's okay to merge this approach. I wonder if we could get the help of some iOS UI dev 🤓
The library hasn't published an update in 7 years, I doubt there will be an update now to fix this compiling issue.
f8ca022
to
a3a43be
Compare
Generated by 🚫 dangerJS |
The library hasn't published an update in 7 years, I doubt there will be an update now to fix this compiling issue.
Regression Notes
Potential unintended areas of impact
None.
What I did to test those areas of impact (or what existing automated tests I relied on)
None.
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.UI Changes testing checklist: N/A