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

Feature/HTML support in profile #99

Merged
merged 9 commits into from Sep 14, 2017
Merged

Conversation

eadm
Copy link
Collaborator

@eadm eadm commented Sep 7, 2017

YouTrack task: #APPS-1457

Description List:

  • Add support of html code to ProfileFragment in user description section
  • Content displayed with WebView but if it is not needed content will be displayed with TextView

@eadm eadm added the bug label Sep 7, 2017
@eadm eadm self-assigned this Sep 7, 2017
@eadm eadm changed the title Feature/html support in profile Feature/HTML support in profile Sep 7, 2017
@KirillMakarov
Copy link
Contributor

Due to changes at CI some gradle tasks were changed. I will fix it for this PR.

@codecov
Copy link

codecov bot commented Sep 13, 2017

Codecov Report

Merging #99 into development will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff                @@
##             development     #99      +/-   ##
================================================
- Coverage           1.68%   1.68%   -0.01%     
  Complexity           122     122              
================================================
  Files                497     497              
  Lines              19381   19398      +17     
  Branches            2766    2770       +4     
================================================
  Hits                 327     327              
- Misses             18981   18998      +17     
  Partials              73      73
Impacted Files Coverage Δ Complexity Δ
...tepic/droid/ui/custom/LatexSupportableWebView.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rc/main/java/org/stepic/droid/util/HtmlHelper.java 37.64% <0%> (-1.38%) 9 <0> (ø)
...a/org/stepic/droid/ui/fragments/ProfileFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba1e390...5a15193. Read the comment docs.

Copy link
Contributor

@KirillMakarov KirillMakarov left a comment

Choose a reason for hiding this comment

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

The web doesn't support LaTeX in the profile info. And we should not use the LaTeX webview for showing the profile info. The best appropriate solution here is to use TextResolver#fromHtml method and simple textview. It will be lightweight and cover 99.9% cases of the platform.

@@ -283,7 +286,19 @@ class ProfileFragment : FragmentBase(),
shortBioFirstHeader.setText(R.string.short_bio_and_info)
shortBioFirstText.text = shortBio
shortBioSecondHeader.setText(R.string.user_info)
shortBioSecondText.text = information

val (description, isNeedWebView, _) = textResolver.resolveStepText(information)
Copy link
Contributor

Choose a reason for hiding this comment

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

information can be in shortBioFirstText, when it is not blank, but shortBio is blank

@@ -35,6 +36,8 @@ import org.stepic.droid.ui.util.TimeIntervalUtil
import org.stepic.droid.ui.util.initCenteredToolbar
import org.stepic.droid.util.AppConstants
import org.stepic.droid.util.ProfileSettingsHelper
import org.stepic.droid.util.resolvers.CoursePropertyResolver
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it used here?

tools:text="Lalala it is info"
tools:visibility="visible"/>
android:paddingTop="4dp">
<org.stepic.droid.ui.custom.LatexSupportableWebView
Copy link
Contributor

Choose a reason for hiding this comment

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

use LatexSupportableEnhancedFrameLayout. It is encapsulation of this logic

@codecov-io
Copy link

codecov-io commented Sep 14, 2017

Codecov Report

Merging #99 into development will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff                @@
##             development     #99      +/-   ##
================================================
- Coverage           1.68%   1.68%   -0.01%     
  Complexity           122     122              
================================================
  Files                497     497              
  Lines              19381   19386       +5     
  Branches            2766    2772       +6     
================================================
  Hits                 327     327              
- Misses             18981   18986       +5     
  Partials              73      73
Impacted Files Coverage Δ Complexity Δ
...p/src/main/java/org/stepic/droid/fonts/FontType.kt 0% <ø> (ø) 0 <0> (ø) ⬇️
...ui/custom/LatexSupportableEnhancedFrameLayout.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...n/java/org/stepic/droid/fonts/FontsProviderImpl.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...tepic/droid/ui/custom/LatexSupportableWebView.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...rc/main/java/org/stepic/droid/util/HtmlHelper.java 39.02% <0%> (ø) 9 <0> (ø) ⬇️
...a/org/stepic/droid/ui/fragments/ProfileFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba1e390...cbfa343. Read the comment docs.

String coloredText = "<font color='" + hexColor + "'>" + text + "</font>";
setTextWebViewOnlyForLaTeX(coloredText);
String coloredText = "<font color='" + hexColor + "'>" + textResult.getText() + "</font>";
setTextWebView(coloredText, HtmlHelper.hasLaTeX(text) && allowLaTeX, fontPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

allowLaTeX is more simple check, please reorder and make it the first

@@ -153,19 +155,22 @@ private static Long parseCourseIdFromNotification(String htmlRaw) {


public static String buildMathPage(CharSequence body, int widthPx, String baseUrl) {
@SuppressLint("DefaultLocale")
Copy link
Contributor

Choose a reason for hiding this comment

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

use Locale.getDefault() instead of SupressLint

String preBody = String.format(PRE_BODY, MathJaxScript, DefaultFontStyle, widthPx, baseUrl);
String result = preBody + body + POST_BODY;
return result;
}

public static String buildPageWithAdjustingTextAndImage(CharSequence body, int widthPx, String baseUrl) {
@SuppressLint("DefaultLocale")
Copy link
Contributor

Choose a reason for hiding this comment

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

use Locale.getDefault() instead of SupressLint

String preBody = String.format(PRE_BODY, " ", RobotoLightFontStyle, widthPx, baseUrl);
public static String buildPageWithCustomFont(CharSequence body, String fontPath, int widthPx, String baseUrl) {
@SuppressLint("DefaultLocale")
String preBody = String.format(PRE_BODY, " ", String.format(CustomFontStyle, fontPath), widthPx, baseUrl);
Copy link
Contributor

Choose a reason for hiding this comment

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

use Locale.getDefault() instead of SupressLint

@@ -218,7 +217,7 @@
fontPath="fonts/Roboto-Light.ttf"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:lineSpacingExtra="3.3sp"
android:lineSpacingMultiplier="1.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

why have we changed it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to match with WebView line spacing which is 1.6 em

android:paddingBottom="@dimen/guideline_standard_padding"
android:paddingLeft="32dp"
android:paddingRight="32dp"
android:paddingTop="4dp"
android:textSize="14sp"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set 14sp to the new view too. For example, we can create custom attr for LatexSupportableEnhancedFrameLayout

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is set in ProfileFragment#onViewCreated

@eadm eadm merged commit 001e052 into development Sep 14, 2017
@eadm eadm deleted the feature/html_support_in_profile branch September 14, 2017 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants