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

Add a workaround for a possible null offset ID. #5067

Merged
merged 1 commit into from
Dec 10, 2020
Merged

Add a workaround for a possible null offset ID. #5067

merged 1 commit into from
Dec 10, 2020

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Dec 2, 2020 •
edited
Loading

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Works around an issue reported by @opusforlife2 in which the ID of the UTC ZoneOffset might be null (even though its documentation explicitly says that a ZoneId's ID value is not null).

APK testing

app-release.zip

Due diligence

@Isira-Seneviratne Isira-Seneviratne mentioned this pull request Dec 2, 2020
Merged
5 tasks
@opusforlife2
Copy link
Collaborator

Debug builds always work (I tried this one as well just to be certain) so I need a release build to test, remember?

@Isira-Seneviratne
Copy link
Member Author

Debug builds always work (I tried this one as well just to be certain) so I need a release build to test, remember?

Right, my bad. I'll upload one now.

@TobiGr TobiGr added the bug Issue is related to a bug label Dec 2, 2020
@opusforlife2
Copy link
Collaborator

Works! Fixed for real this time! (Hopefully xD )

@Isira-Seneviratne
Copy link
Member Author

Works! Fixed for real this time! (Hopefully xD )

Maybe you could report the bug here?

@opusforlife2
Copy link
Collaborator

It could be a bug in LineageOS 16.

XiangRongLin
XiangRongLin reviewed Dec 3, 2020
View reviewed changes
Copy link
Collaborator

@XiangRongLin XiangRongLin left a comment

Choose a reason for hiding this comment

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

I feel like a bug fix like this, which which does not contain any UI stuff, should be covered by a unit tests.
When the underlying bug is fixed and the workaround is not needed anymore, it provides an easy way to verify that it still behaves as expected.

XiangRongLin
XiangRongLin requested changes Dec 3, 2020
View reviewed changes
app/src/main/java/org/schabi/newpipe/util/Localization.java Outdated
@@ -328,7 +328,7 @@ public static String relativeTime(final Calendar calendarTime) {
// This method is a modified version of GregorianCalendar.from(ZonedDateTime).
// Math.addExact() and Math.multiplyExact() are desugared even though lint displays a warning.
@SuppressWarnings("NewApi")
private static GregorianCalendar fromUTC(final OffsetDateTime offsetDateTime) {
public static GregorianCalendar fromUTC(final OffsetDateTime offsetDateTime) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't make a private method public for the sake of testing.

Instead you should test the methods thats using the private method or extract the method into it's own class (not 100% sure on the second part myself yet)

In this case i would write tests for public static String relativeTime(final OffsetDateTime offsetDateTime). If done this way you will provide the value of having regression test for when the workaround is removed. Because currently your tests will need to be removed, when the workaround ist removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried creating an instrumented test, but attempting to run it results in an error message saying that the test class cannot be found.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll open an pr to your branch tonight/tomorrow with what i mean. Maybe i missed something, that makes what i mean not possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I get that you want to test Localization.relativeTime() directly, but I can't run the instrumented test, so I tested PrettyTime manually.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, now i understand why you need an instrumented test. The dependency are direcly accessed and not injected. This would need some major refactoring, which can be done in another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a PR with Dagger, so I could add a commit for DI with Localization there.

XiangRongLin
XiangRongLin requested changes Dec 4, 2020
View reviewed changes
app/src/main/java/org/schabi/newpipe/util/Localization.java Outdated
@@ -314,13 +318,33 @@ private static void initPrettyTime(final Context context) {
}

public static String relativeTime(final OffsetDateTime offsetDateTime) {
return relativeTime(GregorianCalendar.from(offsetDateTime.toZonedDateTime()));
return relativeTime(fromUTC(offsetDateTime));
Copy link
Collaborator

@XiangRongLin XiangRongLin Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

The tests are not covering the bug that you fixed. I can change back this line and your tests still pass successfully

nvm, i realised my mistake.

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Dec 5, 2020 via email •
edited
Loading

@Isira-Seneviratne Isira-Seneviratne force-pushed the Add_workaround_for_null_offset_ID branch 3 times, most recently from b9138bc to ca30c7a Compare December 10, 2020 09:39
@Isira-Seneviratne
Copy link
Member Author

@opusforlife2 could you test the new APK? I moved the workaround to OffsetDateTime.toCalendar() so that this issue won't potentially affect FeedViewModel.

TobiGr
TobiGr approved these changes Dec 10, 2020
View reviewed changes
Copy link
Member

@TobiGr TobiGr left a comment

Choose a reason for hiding this comment

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

thanks again

@TobiGr TobiGr merged commit 3c306a0 into TeamNewPipe:dev Dec 10, 2020
@Isira-Seneviratne Isira-Seneviratne deleted the Add_workaround_for_null_offset_ID branch December 10, 2020 10:39
@opusforlife2
Copy link
Collaborator

@Isira-Seneviratne 0.20.6 works perfectly fine, so your modification seems to be okay.

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

@XiangRongLin XiangRongLin XiangRongLin requested changes

@TobiGr TobiGr TobiGr approved these changes

Assignees
No one assigned
Labels
bug Issue is related to a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Isira-Seneviratne @opusforlife2 @TobiGr @XiangRongLin