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

[Issue #1] Application crashes (immediately) after choosing 'View log' from the menu. #1337

Open
wants to merge 4 commits into
base: beta
Choose a base branch
from

Conversation

amaa-99
Copy link

@amaa-99 amaa-99 commented Jul 25, 2023

Changed the required version to org.ocpsoft.prettytime:prettytime to 4.0.6.Final (as per the library documentation). This should be compatible with all the target OS versions.

@amaa-99 amaa-99 changed the title * [Issue #1] Application crashes (immediately) after choosing ' View … [Issue #1] Application crashes (immediately) after choosing ' View … Jul 25, 2023
@amaa-99 amaa-99 changed the title [Issue #1] Application crashes (immediately) after choosing ' View … [Issue #1] Application crashes (immediately) after choosing ' View log' from the menu. Jul 25, 2023
@amaa-99 amaa-99 changed the title [Issue #1] Application crashes (immediately) after choosing ' View log' from the menu. [Issue #1] Application crashes (immediately) after choosing 'View log' from the menu. Jul 25, 2023
Copy link
Owner

@ukanth ukanth left a comment

Choose a reason for hiding this comment

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

Rather than reverting to older version, can you please explain from which class the issue occurs ? we can try fixing with latest version of prettytime.

@amaa-99
Copy link
Author

amaa-99 commented Jul 28, 2023

Thanks for having a look.

Please refer to to original issue on my fork:
amaa-99#1

"This is due to the included version of the prettytime library using java 1.8 features which are not available in these OS versions.
As per the library documentation (https://github.com/ocpsoft/prettytime) version 4.0.6.Final or lower is to be used in these cases."

@ukanth
Copy link
Owner

ukanth commented Jul 29, 2023

Java 1.8 (Java 8( is not Java 18. It should technically work with latest/old Android versions. I would like to fix the actual issue with 5.x version rather than downgrade it to 4.x

@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch from 85f2e05 to c4732ab Compare July 29, 2023 21:38
@amaa-99
Copy link
Author

amaa-99 commented Jul 29, 2023

I've removed the org.ocpsoft.prettytime:prettytime (which was used in a single spot to format a string), and replaced the implementation to use standard APIs.
(I've removed the related unused code and declarations which made it seem that that was used more often.)

Calendar calendar = Calendar.getInstance();
calendar.setTimeInMillis(data.getTimestamp());
DateFormat dateFormat = DateFormat.getDateTimeInstance(DateFormat.SHORT, DateFormat.SHORT);
holder.lastDenied.setText(dateFormat.format(calendar.getTime()));
Copy link
Author

Choose a reason for hiding this comment

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

This is the only place where that is used.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for PR. Does it retain the same functionality as prettytime ?

Copy link
Author

Choose a reason for hiding this comment

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

That's one of the java standard ways of printing a localized date/time (taking the time zone and ... into consideration).
It should do what's needed for the date/time string shown in the log view.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually it was not showing just data and time .It was showing the time lapse like 2s ago, 10s ago, 1 min ago etc.,

Copy link
Author

Choose a reason for hiding this comment

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

Note: In the detail view of the log entries the date/time is shown without any localization.
Probably the localized time should be shown there too.

Copy link
Author

Choose a reason for hiding this comment

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

I think the focus should be kept on fixing the crash first without adding complexity.
The pretty format can always be added on top...
I don't have much passion on which direction the solution goes, but I find it a pity having the app crash for something like that.
In my opinion as long as one can see the date/time of the logged event it's perfect (and it's even localized, so the US won't be mixing months with days ;)). What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Agree with your point. But I don't see any crashing reports based on this.

if (info != null && info.applicationInfo != null) {
Drawable drawable = info.applicationInfo.loadIcon(manager);
holder.icon.setBackground(drawable);
} else {
Drawable appIcon = context.getDrawable(R.drawable.ic_unknown);
holder.icon.setImageBitmap(Api.getBitmapFromDrawable(appIcon));
Copy link
Author

@amaa-99 amaa-99 Jul 30, 2023

Choose a reason for hiding this comment

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

The

holder.icon.setBackground(drawable);

is not consistent with the similar code a few lines below:

if (Build.VERSION.SDK_INT > android.os.Build.VERSION_CODES.O) {
    holder.icon.setBackground(context.getDrawable(R.drawable.ic_unknown));
} else {
    Drawable appIcon = context.getDrawable(R.drawable.ic_unknown);
    holder.icon.setImageBitmap(Api.getBitmapFromDrawable(appIcon));
}

At first glance it would seem that

holder.icon.setImageBitmap(Api.getBitmapFromDrawable(drawable))

should be used at that spot instead.

@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch 2 times, most recently from 6d7f3dc to f65f3ef Compare July 30, 2023 09:16
@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch from f65f3ef to a2c6320 Compare August 9, 2023 17:45
@ukanth
Copy link
Owner

ukanth commented Aug 14, 2023

I will accept this PR if you handle the exception thrown by prettytime and fall back to normal timestamp rather than completely removing the feature.

@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch from a2c6320 to f71e192 Compare August 19, 2023 16:14
@amaa-99
Copy link
Author

amaa-99 commented Aug 19, 2023

Could you follow-up on this one? Just cherry pick what's useful into your branch and add the rest...
The issue can be reproduced in the device emulators for the Android 5 and 6: You just need to open the log view with some entries in there.
(Alternatively just add a call to those PrettyTime methods somewhere, e.g. in the creation of the apps list, to see the exception).

@amaa-99
Copy link
Author

amaa-99 commented Aug 19, 2023

p.s. This seems to be a duplicate of the [(https://github.com//issues/1344)] issue.

@ukanth
Copy link
Owner

ukanth commented Aug 20, 2023

Could you follow-up on this one? Just cherry pick what's useful into your branch and add the rest... The issue can be reproduced in the device emulators for the Android 5 and 6: You just need to open the log view with some entries in there. (Alternatively just add a call to those PrettyTime methods somewhere, e.g. in the creation of the apps list, to see the exception).

Thanks. As indicated above, please add an exception to handle rather than removing prettytime all together. I will merge the PR once that's done.

@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch from f71e192 to e2871dd Compare September 22, 2023 10:43
…log' from the menu.

Dropped the use of the org.ocpsoft.prettytime:prettytime library: This fixes the crashes due to the library using unsupported language features on older platforms.
@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch from e2871dd to 4c3a7b3 Compare September 22, 2023 11:19
amaa-99 added 3 commits September 22, 2023 17:51
…log' from the menu.

Addressed a couple of lint warnings.
…iews.

Corrected the date/time format in the details view (e.g. 19:00 would get displayed as '7:00').
Changed the detail view to not show the host name field in case the host name is not known.
…iews.

Removed commented out code and unused methods.
@amaa-99 amaa-99 force-pushed the 1-application-crashes-immediately-after-choosing-view-log-from-the-menu branch from 4c3a7b3 to a5f1f16 Compare September 22, 2023 15:56
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