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

Using System.getProperty("line.separator") instead of '\n' and adding pre-caching of licenses text #35

Merged
merged 4 commits into from
Oct 3, 2014

Conversation

ngdelamo
Copy link
Contributor

@ngdelamo ngdelamo commented Oct 3, 2014

In some Android versions using '\n' does not lead to a new line on TextViews. Instead, the standard System.getProperty("line.separator") must be used. This was affecting licenses text, since the method for reading them was adding '\n' characters that were not being shown in the licenses dialog (see the following image from a screenshot on a Nexus 5 running Android 4.4.4)
licenses_dialgo_line_break

Also, when a license's text was requested, either by using 'getSummaryText' or 'getFullText', a fresh new read from the raw resource was performed. This can slow down the dialog when multiple instances of the same license are used, which is quite common (most licenses on our projects are apache 2.0). I added some changes to read the raw resource once and store the resulting String, so subsequent calls to the previous methods would return the cached String.

@@ -64,9 +64,10 @@ protected String getContent(final Context context, final int contentResourceId)

private String toString(final BufferedReader reader) throws IOException {
final StringBuilder builder = new StringBuilder();
String lineSeparator = System.getProperty("line.separator");
Copy link
Member

Choose a reason for hiding this comment

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

Make that a constant

@hameno
Copy link
Member

hameno commented Oct 3, 2014

Thank you for your contribution. Could you please take a look at my comments and fix them? Also, I feel like the caching mechanism produces a lot of duplicate code, maybe you could move it to the base class?

@ngdelamo
Copy link
Contributor Author

ngdelamo commented Oct 3, 2014

Sure, I'll add the changes. I was not sure about adding the cached Strings to the License class in case you wanted to keep it implementation-independent. But if you don't mind, I'll try to pull the cache code to the parent License class

@hameno
Copy link
Member

hameno commented Oct 3, 2014

That would be better I think :)

@ngdelamo
Copy link
Contributor Author

ngdelamo commented Oct 3, 2014

Done. Could you review my changes? I had to rename some methods in the license subclasses in order to maintain compatibility with the previously existing ones in the License base class.

@hameno
Copy link
Member

hameno commented Oct 3, 2014

Done, just three little things and then it's good to go.

…ext re-defined to be final in order to prevent overriding
hameno added a commit that referenced this pull request Oct 3, 2014
Using System.getProperty("line.separator") instead of '\n' and adding pre-caching of licenses text
@hameno hameno merged commit 1c3f724 into PSDev:master Oct 3, 2014
@ngdelamo
Copy link
Contributor Author

ngdelamo commented Oct 3, 2014

Good, that was a pretty fast PR :)

@hameno
Copy link
Member

hameno commented Oct 3, 2014

Yep, holiday here ;)

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