Closed Bug 1212596 Opened 6 years ago Closed 6 years ago

Highlight Remove account preference in red color text

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: vivek, Assigned: martianwars, Mentored)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(2 files, 12 obsolete files)

6.03 KB, patch
vivek
: review+
nalexander
: review+
Details | Diff | Splinter Review
6.04 KB, patch
Details | Diff | Splinter Review
This is a follow up to 1205817. 
We would like to highlight the disconnect button in "rejection_red" color.

There are three ways update the text color for Preference in Android.

1) Use custom layout like we do for here message [1]
2) Extend Preference class, we can customize this to change color for 
   both text and summary
3) Use spannables as mentioned here [2] (feels like hack)

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml#44
[2] http://goo.gl/jvvVBc
Kalpesh,

You did a great job with bug 1205817. 
Are you interested in working on this follow up bug.
I will comment with more details on the approach to solve this by tomorrow.
Flags: needinfo?(kalpeshk2011)
Yeah I would be interested :)
Flags: needinfo?(kalpeshk2011)
Kalpesh,

Thanks for your interest.
To solve this bug lets use method 1 from https://bugzilla.mozilla.org/show_bug.cgi?id=1212596#c0.
I feel the third options is very hacky and I don't see any need for customizable Preference.

I have given reference to custom layouts in the above comment.
Please let me know if something is not clear.
Mentor: nalexander, vivekb.balakrishnan
Attached patch fxremoveaccountcolor.patch (obsolete) — Splinter Review
Do I need to make a new custom layout? The custom layout you have hyperlinked seems to be highlighting in "rejection_red"
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8672199 - Flags: review?(vivekb.balakrishnan)
Kalpesh,

Sorry for the delay in response. Yes, you need to create a new custom layout.
The referenced layout was meant as an example.

For our case we would need a custom layout that has said red color text for "Disconnect..." . 
Also, note that the styles should match otherwise with existing preference.

Please let me know if anything is not clear.
Flags: needinfo?(vivekb.balakrishnan)
Comment on attachment 8672199 [details] [diff] [review]
fxremoveaccountcolor.patch

Review of attachment 8672199 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,

I'm closing this review request.
To solve this bug, we need to define a new custom layout that has text color in red.
Also, we don't need icons for this Disconnect preference.

Please NI me if you need more info.
Attachment #8672199 - Flags: review?(vivekb.balakrishnan) → review-
Attached patch colorfix.patch (obsolete) — Splinter Review
Attachment #8672199 - Attachment is obsolete: true
Attachment #8673152 - Flags: review?(vivekb.balakrishnan)
Assignee: nobody → kalpeshk2011
Comment on attachment 8673152 [details] [diff] [review]
colorfix.patch

Review of attachment 8673152 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,

This patch is missing fxaccount_remove_account.xml
When you add a new file you need to do "hg add file" for mercurial to track it.

Can you please re-submit a new patch.

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +105,5 @@
>          <Preference
>              android:editable="false"
>              android:key="remove_account"
>              android:persistent="false"
> +            android:layout="@layout/fxaccount_remove_account"

Also, lets maintain parity with other layout preferences.
can you please rename this to something like fxaccount_remove_account_preference
Attachment #8673152 - Flags: review?(vivekb.balakrishnan) → review-
Attached patch colorfix.patch (obsolete) — Splinter Review
I hope it's better now.
Attachment #8673152 - Attachment is obsolete: true
Attachment #8673363 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8673363 [details] [diff] [review]
colorfix.patch

Review of attachment 8673363 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,

This is a really good attempt but we need to improve upon the styling.
Styling this text item will require lots of iteration to get it right.
Instead, we could simply copy the preference layout from android source and then set the text color for title item [1].
This is lame but it is required for this bug.

So the solution for this patch :
1) copy the layout from [1]
2) fix all styling issues that IDE complains
3) set textcolor and text for the title view 
4) set the icon, summary and widget_frame to visibility gone.

Also, note that when you create a new file you always add the Mozilla Public license comment.
For xml files this must be immediately after the xml tag. 
For java/js files this must be very first line even before package declaration. 


Thanks for your effort :)

[1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/res/res/layout/preference.xml

::: mobile/android/base/resources/layout/fxaccount_remove_account_preference.xml
@@ +4,5 @@
> +    android:layout_height="match_parent">
> +    <TextView
> +        android:id="@+android:id/title"
> +        style="@style/FxAccountTextItem"
> +        android:layout_width="fill_parent"

This is not a review, just a self note.
While declaring the custom layout always define icon, summary and widget_frame which may be accessed programmatically in java code.
You can set the visibility of these elements to gone.
Attachment #8673363 - Flags: review?(vivekb.balakrishnan) → feedback+
Attached patch colorfix.patch (obsolete) — Splinter Review
Hey Vivek, did as you said. I hope it looks okay now
Attachment #8673363 - Attachment is obsolete: true
Attachment #8674367 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8674367 [details] [diff] [review]
colorfix.patch

Review of attachment 8674367 [details] [diff] [review]:
-----------------------------------------------------------------

Kalpesh,

Thanks you for your effort.
I have a few minor nits and one important license comment issue.

I also noticed that the text is a little bigger in comparison (may be it is just me) and it is not aligned with other preferences.
I need time to investigate why it is not aligned correctly. 
I am leaving the review flag to get back to this alignment issue.

Can you please post a screenshot and ask for review from Nick.
While you do that can you post another screenshot with text size set to @style/TextAppearance.Medium
You can post the screenshot as a patch or post a link to your shared dropbox or google drive folder

Feel free to NI me if you need any info or ping me in irc. My irc nickname is vivek
I am usually signed in #mobile channel.

::: mobile/android/base/resources/layout/fxaccount_remove_account_preference.xml
@@ +1,4 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

As we copied this file from android source, don't change the license comment to MPL. That is copy it as it is.

@@ +6,5 @@
> +<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
> +android:layout_width="match_parent"
> +android:layout_height="wrap_content"
> +android:minHeight="?android:attr/listPreferredItemHeight"
> +android:gravity="center_vertical"

Please indent with 4 spaces.
Also align the start of android: attribute so that they are aligned with xmlns: attribute
And use line break to separate different element, improves readability :)

@@ +15,5 @@
> +    android:layout_height="wrap_content"
> +    android:visibility="gone"
> +    android:layout_gravity="center" />
> +<RelativeLayout
> +    android:layout_width="wrap_content"

use 0dp instead of wrap_content here
@vivek :- I'm having issues (like I mentioned in the previous bug) in running Firefox for Android. How do I detect the strings in my project? The project won't build without that. 
Also, is the testing procedure identical to Android apps, where I use USB Debugging to run the app, or am I supposed to run some form of unit / integrated tests like done in other Mozilla projects like Gaia, Toolkit?
Flags: needinfo?(vivekb.balakrishnan)
@Kalpesh, 

can you post a detailed error log in pastebin.mozilla.org? 
Also can you please let me know how you are building fennec, say after adding a string.
BTW, are you using IntelliJ for development? I think you should, it is awesome :)

I usually just use ctr + click key-combo (Linux) in IntelliJ to open the generated string file.
Note, this file is generated and I use it just for checking the string values. If you edit it, it will lost when you build again.

I am not aware what Gaia or toolkit development process is. Ideally, you should run tests to verify that your changes does not break things.
We have unit tests but it is mostly related to background Sync stuff.
We have robocop tests that runs the tests in your phone through USB [1] but running a complete suite takes like half an hour.

I'm usually happy with manual testing. Robocop tests are run in try server anyway ;)

As I said early, please try to hang out in mobile irc channel. The people there are very friendly and somebody will try help to you out even if I am offline.

[1] https://wiki.mozilla.org/Mobile/Fennec/Android/Testing
Flags: needinfo?(vivekb.balakrishnan)
@vivek,

Well I start my IDE (Android Studio) using the command `sudo ./mach ide androidstudio`
It takes ~4-5 minutes to open up. Everything is fine except the strings. I keep getting the @string references highlighted in red due to a missing strings.xml file. As a result, I can't really test my work.

If it isn't clear, I'll post links to pictures of my screen in my next comment.

I have asked my query on the IRC as well. In the case I figure out the correct procedure before your reply, I will post the updated patch :)
Flags: needinfo?(vivekb.balakrishnan)
@vivek :- Should I switch to IntelliJ IDEA? I had the wrong impression that IntelliJ and AndroidStudio are exactly the same thing.
(In reply to Kalpesh Krishna from comment #16)
> @vivek :- Should I switch to IntelliJ IDEA? I had the wrong impression that
> IntelliJ and AndroidStudio are exactly the same thing.

They should work about the same.  I think your issues should be fixed (with strings.xml) if you run |mach build mobile/android| and then in the IDE, choose the Gradle view (View > Tool windows > Gradle) and refresh.  That should get the strings in place.

Unfortunately, I don't think you can see the strings in the design view -- see Bug 1074258.
I don't think it is working. This is how my screen looks after refreshing Gradle :- http://picpaste.com/Screenshot_from_2015-10-17_01_56_52-7n6obwv5.png

Would running the command |mach build| and selecting "Firefox for Android" produce a different result when compared to |mach build mobile/android| ? I've done the former
Flags: needinfo?(nalexander)
Flags: needinfo?(vivekb.balakrishnan)
Well I got the app to work on my phone after a successful build :) Sorry for the trouble. Do let me know whether the picpaste.com image is something to worry about. I will submit the new patch with the screenshots soon.
Attached patch colorfix.patch (obsolete) — Splinter Review
Well this method seems to give a good result. Posting screenshots in subsequent posts
Attachment #8674367 - Attachment is obsolete: true
Attachment #8674367 - Flags: review?(vivekb.balakrishnan)
Attachment #8675139 - Flags: review?(vivekb.balakrishnan)
Attached patch How Disconnect... looks. (obsolete) — Splinter Review
Attachment #8675143 - Flags: review?(nalexander)
(In reply to Kalpesh Krishna from comment #18)
> I don't think it is working. This is how my screen looks after refreshing
> Gradle :-
> http://picpaste.com/Screenshot_from_2015-10-17_01_56_52-7n6obwv5.png
> 
> Would running the command |mach build| and selecting "Firefox for Android"
> produce a different result when compared to |mach build mobile/android| ?
> I've done the former

Kalpesh -- I think you meant "|mach bootstrap| and selecting "Firefox for Android"", not mach build.  That configures your environment but doesn't actually do the build.  The |mach build mobile/android| after that will update the files, including making sure strings.xml gets generated.
Flags: needinfo?(nalexander)
Comment on attachment 8675139 [details] [diff] [review]
colorfix.patch

Review of attachment 8675139 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think a custom preference layout is necessary: I think you should be able to do this with a custom preference class and the regular layout.  Then you just update the text color in code, and don't worry about the spacing, etc.  (The title IDs are guaranteed to be fixed since Android expects them.)

See http://stackoverflow.com/a/14054989.
Comment on attachment 8675143 [details] [diff] [review]
How Disconnect... looks.

Let's try to get the visual style just right -- see my earlier comment about using a custom preference, not custom layout.
Attachment #8675143 - Flags: review?(nalexander) → feedback+
Hi Kalpesh,

I'm sorry to have lead you down the wrong path with custom preference layouts.
I was convinced earlier that this will be a simpler fix with custom layouts but seems like 
it is actually quite difficult to get the visual styling right. 
At least this was a good learning experience for me. Hope it was same for you.

As Nick demonstrated in the previous comment, we can solve this bug with custom preference class .

Are you still interested in seeing this bug to completion?

Please feel free to NI/ping me in case if you have some queries.
Flags: needinfo?(kalpeshk2011)
Yes absolutely! Contributing brings cheer up my day :D This was a good learning experience for me as well. 
I have a few queries though :- 
1. What was the exact issue with the custom layout? The latest patch I sent you had the appropriate text size and style (when compared to other menu options) in my opinion.
2. Could you tell me about some files implementing a custom preference which will assist me while I re-work on this bug? Or an online guide to the same. 

Hoping to do a good job!
Flags: needinfo?(kalpeshk2011) → needinfo?(vivekb.balakrishnan)
Kalpesh,

Thanks for asking the right questions.

1) The issue with custom layout is that the android has two variants of preference layout -> one we copied and the another for holo themes named preference_holo [1]. Preference_holo defines a padding left on the top level linear layout of 8dip which is not there in other regular layout. Also, the text appearance is textAppearanceMedium not textAppearanceLarge. 

In Fennec, on pre-21 android devices preferences inherit from Themes.holo.Light style [2]. So, that's why was I was having the preference misalignment  with my Kitkat device while you didn't have any such issue with your lollypop. Here is a screen grab from my phone for the regular and holo layouts [3].

This google group discussion covers on this issue [4].

Once again thanks for asking this question, this was good learning experience for me :).
Please let me know if something was not clear.

I will answer your second question as a separate comment.


[1] https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/res/res/layout/preference_holo.xml#25

[2] http://mxr.mozilla.org/mozilla-central/search?string=GeckoPreferencesBase&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

[3] https://www.dropbox.com/sh/fszv7xw4qfmu5f1/AACNNvF1ZO5ReFre_38C2p-ha?dl=0

[4] https://groups.google.com/forum/#!topic/android-developers/R8DrriOnuGQ
Flags: needinfo?(vivekb.balakrishnan)
Hi Vivek, yeah it's pretty much clear now. Thanks for the effort. 
Eagerly waiting for the second answer.
@Kalpesh,

Please refer CustomCheckBoxPreference class for pointer on how to solve this bug. [1]

Lets us not hard code the color info in Java file. So define some custom attributes and apply the text color in onBindView() [2]. Also, Lets do this correct so that it is reusable i.e apply separate colors for both title and summary.

As always please feel free to NI me if something is not clear.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/CustomCheckBoxPreference.java#21
[2] http://developer.android.com/training/custom-views/create-view.html#customattr
Flags: needinfo?(kalpeshk2011)
Attached patch colorfix.patch (obsolete) — Splinter Review
Hey Vivek, I tried to use the links you sent me. I guess I still have to tell the Preference about this custom layout class. I saw a few examples doing them via Java. How could I do the same? Also, do let me know the corrections in the patch I'm submitting.
Attachment #8675139 - Attachment is obsolete: true
Attachment #8675139 - Flags: review?(vivekb.balakrishnan)
Flags: needinfo?(kalpeshk2011) → needinfo?(vivekb.balakrishnan)
Attachment #8678346 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8678346 [details] [diff] [review]
colorfix.patch

Review of attachment 8678346 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

I am yet to test your patch on a real device, I will wait for an updated version :)
Please see my review comments below.

In the prefscreen.xml you can directly call this preference with the full qualified package name like 
<org.mozilla.gecko.preferences.RemoveAccountPreference
            android:editable="false"
            android:key="remove_account"
            android:persistent="false"
            gecko:titleColor="@color/rejection_red"
            android:title="@string/fxaccount_remove_account" />

You should declare the xml namespace for gecko in PreferenceScreen element [1].
Check the AlignRightLinkPreference example [2].
It is fine if you want to drop the namespace for these custom attributes like we do in LinkPreference.


[1] http://developer.android.com/training/custom-views/create-view.html#customattr
[2] http://mxr.mozilla.org/mozilla-central/search?string=LinkPreference
Also, note that you need to add the java class to moz.build like here https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/moz.build#417

::: mobile/android/base/preferences/RemoveAccountPreference.java
@@ +1,1 @@
> +package org.mozilla.gecko.preferences;

You forgot the Mozilla license comment :)

@@ +7,5 @@
> +import android.view.View;
> +import android.widget.TextView;
> +
> +
> +class RemoveAccountPreference extends Preference {

Please rename this class to CustomColoredPreference or something similar.

@@ +13,5 @@
> +    private int mTitleColor, mSummaryColor;
> +    public RemoveAccountPreference(Context context) {
> +        super(context);
> +    }
> +    public RemoveAccountPreference(Context context, AttributeSet attrs) {

Please add the third constructor variant with (Context context, AttributeSet attrs, int defStyle). 
Also, to avoid code duplication you move the come code to a init() function.

@@ +24,5 @@
> +
> +    @Override
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +        title = (TextView) view.findViewById(android.R.id.title);

I am not convinced that a null check is needed here, so I guess this is fine.

::: mobile/android/base/resources/values/attrs.xml
@@ +167,5 @@
>          <attr name="inactiveTextColor" format="color" />
>          <attr name="titlebarFill" format="boolean" />
>      </declare-styleable>
>  
> +    <declare-styleable name="RemoveAccountPreference">

Change CustomColorPreference or something similar
Attachment #8678346 - Flags: review?(vivekb.balakrishnan) → a11y-review+
@Kalpesh,
Please attach a screen grab with you patch and ask review from Nick
Flags: needinfo?(vivekb.balakrishnan) → needinfo?(kalpeshk2011)
Attachment #8678346 - Flags: a11y-review+ → feedback+
Well I am getting the following error in my Android Studio :- 

[ 504769]   WARN - g4idea.command.HgStatusCommand - Could not execute hg status command [abort: path 'obj-arm-linux-androideabi/mobile/android/gradle/base/src/main/java/org/mozilla/gecko/preferences/RemoveAccountPreference.java' traverses symbolic link 'obj-arm-linux-androideabi/mobile/android/gradle/base/src/main/java/org/mozilla/gecko'] 

Can't really build the project due to this.
Flags: needinfo?(vivekb.balakrishnan)
Well slight error above, the error message is shown here :- https://pastebin.mozilla.org/8850302
Flags: needinfo?(kalpeshk2011)
Attached patch colorfix.patch (obsolete) — Splinter Review
I haven't really tested it due to the Gradle build error. I have built my project using |sudo mach build mobile/android|
Attachment #8678346 - Attachment is obsolete: true
Attachment #8678449 - Flags: review?(vivekb.balakrishnan)
Attached patch colorfix.patch (obsolete) — Splinter Review
Sorry, messed up :/ Had forgotten to add the renamed file.
Attachment #8678449 - Attachment is obsolete: true
Attachment #8678449 - Flags: review?(vivekb.balakrishnan)
Attachment #8678450 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8678450 [details] [diff] [review]
colorfix.patch

Review of attachment 8678450 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

Your patch looks good except for the couple of changes I have asked for.

Regarding the build failure, You need to add an entry for CustomColorPreference.java in mobile/android/base/moz.build file. 
This is the metadata file for the build process and I suspect this is the root cause for your build failure.

Please address the following review comments.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +10,5 @@
> +import android.util.AttributeSet;
> +import android.view.View;
> +import android.widget.TextView;
> +
> +import org.w3c.dom.Attr;

Please remove this import.

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +106,3 @@
>              android:editable="false"
>              android:key="remove_account"
>              android:persistent="false"

Please use the custom attribute "titleColor" to define the color.
As a part of this change, you need to add a namespace for the custom attribute

@@ +106,5 @@
>              android:editable="false"
>              android:key="remove_account"
>              android:persistent="false"
>              android:title="@string/fxaccount_remove_account" />
> +

Nit. Remove this new line
Attachment #8678450 - Flags: review?(vivekb.balakrishnan) → feedback+
Comment on attachment 8678450 [details] [diff] [review]
colorfix.patch

Review of attachment 8678450 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +32,5 @@
> +
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.CustomColorPreference);

Just realized that you are also missing the import for R.java
Flags: needinfo?(vivekb.balakrishnan)
My moz.build file :- https://pastebin.mozilla.org/8850949
Flags: needinfo?(vivekb.balakrishnan)
@Kalpesh,

Can you post only the relevant section of your moz.build file. Sorry ,that pastebin link doesn't include your relevant changes.
Flags: needinfo?(vivekb.balakrishnan)
@vivek :- I'm sorry, I copied the wrong file. I've added the entry, I'll attach the updated patch soon and let you know about the build status.
Attached patch colorfix.patch (obsolete) — Splinter Review
I am getting the following build error now :( :- 
https://pastebin.mozilla.org/8850956
Attachment #8678450 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8681256 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8681256 [details] [diff] [review]
colorfix.patch

Review of attachment 8681256 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

You patch did compile fine for me and it did not fail in try as well :). 
Can you please try a clobber build?

Overall the patch looks good except for a few minor nits.
Also, note that the patch did not apply cleanly against fx-team.
Can you please submit a new patch rebased against latest fx-team branch addressing those nits.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko.preferences;
> +import org.mozilla.gecko.R;

Add a new line between package and import

@@ +11,5 @@
> +import android.util.AttributeSet;
> +import android.view.View;
> +import android.widget.TextView;
> +
> +

Can you add a comment explaining what this class is about.
Also, kill the extra lines.

@@ +16,5 @@
> +
> +class CustomColorPreference extends Preference {
> +    private TextView title, summary;
> +    private int mTitleColor, mSummaryColor;
> +    public CustomColorPreference(Context context) {

Add a new line between fields and constructor

@@ +27,5 @@
> +    }
> +
> +    public CustomColorPreference(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        init(context,attrs);

nit: space between arguments

@@ +31,5 @@
> +        init(context,attrs);
> +
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {

nit: space between parameters

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +1,3 @@
>  <?xml version="1.0" encoding="utf-8"?>
>  <PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android"
> +    xmlns:gecko="http://schemas.android.com/apk/res-auto"

Can you please align it so that this line and the next start right below xmlns:android
Attachment #8681256 - Flags: review?(vivekb.balakrishnan) → feedback+
Flags: needinfo?(vivekb.balakrishnan)
Attached patch colorfix.patch (obsolete) — Splinter Review
I hope it is okay now. 
|./mach clobber| worked successfully for me. If this isn't the correct command for the clobber build, do let me know.
Attachment #8681256 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8681788 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8681788 [details] [diff] [review]
colorfix.patch

Review of attachment 8681788 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

Thanks for being patient with my pedantic reviews.
We are almost there, can you please address the following comments.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +12,5 @@
> +import android.view.View;
> +import android.widget.TextView;
> +
> +/**
> + * This class is used to assign the rejection red color to a

change this to something like "This preference is used to define custom colors both title and summary texts. placeholder grey color is used as fallback color for both title and summary."

@@ +33,5 @@
> +
> +    public CustomColorPreference(Context context, AttributeSet attrs, int defStyle) {
> +        super(context, attrs, defStyle);
> +        init(context,attrs);
> +

nit: remove this extra line

@@ +38,5 @@
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {
> +        TypedArray a = context.obtainStyledAttributes(attrs, R.styleable.CustomColorPreference);
> +        mTitleColor = a.getColor(R.styleable.CustomColorPreference_titleColor, R.color.rejection_red);

use placeholder_grey as fallback for title

::: mobile/android/base/resources/xml/fxaccount_status_prefscreen.xml
@@ +1,4 @@
>  <?xml version="1.0" encoding="utf-8"?>
>  <PreferenceScreen xmlns:android="http://schemas.android.com/apk/res/android"
> +                  xmlns:gecko="http://schemas.android.com/apk/res-auto"
> +    android:key="status_screen">

Please align this. Sorry for not mentioning it earlier
Attachment #8681788 - Flags: review?(vivekb.balakrishnan) → feedback+
@vivek :- No it's important to be pedantic. I didn't get what you mean by fallback for title?
Attached patch colorfix.patch (obsolete) — Splinter Review
Fixing the nits
Attachment #8681788 - Attachment is obsolete: true
Attachment #8681983 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8681983 [details] [diff] [review]
colorfix.patch

Review of attachment 8681983 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

Can you please change the class comment as mentioned below and upload a new patch.
Apart from that it looks good to me.
Thanks for your effort.

Also, requesting @Nick for a super review.

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +14,5 @@
> +
> +/**
> + * This preference is used to define custom
> + * colors for both title and summary texts.
> + * Placeholder grey color is used as the 

Can you please replace the entire comment section with this

 /**
 * This preference is used to define custom  colors for both title and summary texts.
 * Color code #777777 (placeholder_grey) is used as the fallback color for both title and summary.
 */

Note that line width for fennec is 120 characters.Also avoid trailing spaces at end of a line.
Attachment #8681983 - Flags: review?(vivekb.balakrishnan)
Attachment #8681983 - Flags: review?(nalexander)
Attachment #8681983 - Flags: review+
Attachment #8682041 - Flags: review?(vivekb.balakrishnan)
Comment on attachment 8682041 [details] [diff] [review]
colorfix.patch with comment update

Review of attachment 8682041 [details] [diff] [review]:
-----------------------------------------------------------------

@Kalpesh,

Thanks for the quick turn around. 
I'll wait for Nick's review before marking landing it.
Attachment #8682041 - Flags: review?(vivekb.balakrishnan)
Attachment #8682041 - Flags: review?(nalexander)
Attachment #8682041 - Flags: review+
@vivek :- That's excellent :) Any good bug I could work on next?
Comment on attachment 8682041 [details] [diff] [review]
colorfix.patch with comment update

Review of attachment 8682041 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good!  A few nits (small details) and one real thing (no need to save the View elements).

::: mobile/android/base/preferences/CustomColorPreference.java
@@ +15,5 @@
> + /**
> + * This preference is used to define custom  colors for both title and summary texts.
> + * Color code #777777 (placeholder_grey) is used as the fallback color for both title and summary.
> + */
> +

nit: no newline.

@@ +17,5 @@
> + * Color code #777777 (placeholder_grey) is used as the fallback color for both title and summary.
> + */
> +
> +class CustomColorPreference extends Preference {
> +    private TextView title, summary;

nit: one field per line.  Subsequent changes (removals) don't touch more than the one element they concern.

@@ +34,5 @@
> +        super(context, attrs, defStyle);
> +        init(context,attrs);
> +    }
> +
> +    public void init(Context context,AttributeSet attrs) {

nit: space after ,.

@@ +44,5 @@
> +
> +    @Override
> +    protected void onBindView(View view) {
> +        super.onBindView(view);
> +        title = (TextView) view.findViewById(android.R.id.title);

There's no need to save these as fields, since they are only set and never read.

::: mobile/android/base/resources/values/attrs.xml
@@ +167,5 @@
>          <attr name="inactiveTextColor" format="color" />
>          <attr name="titlebarFill" format="boolean" />
>      </declare-styleable>
>  
> +    <declare-styleable name="CustomColorPreference">

Nifty!  I learned something.
Attachment #8682041 - Flags: review?(nalexander) → review+
Comment on attachment 8681983 [details] [diff] [review]
colorfix.patch

I've reviewed a later version.
Attachment #8681983 - Flags: review?(nalexander)
@nalexander,

I didn't quite get what you meant by "save these as fields" :(
Flags: needinfo?(nalexander)
(In reply to Kalpesh Krishna from comment #55)
> @nalexander,
> 
> I didn't quite get what you meant by "save these as fields" :(

No need to store them as member variables.  That is, get rid of

 private TextView title, summary;
Flags: needinfo?(nalexander)
Okay so what would I do to 
> title = (TextView) view.findViewById(android.R.id.title);
> summary = (TextView) view.findViewById(android.R.id.summary);
> title.setTextColor(mTitleColor);
> summary.setTextColor(mSummaryColor);

Do I need to setTextColor at all? or should I do it like this :- 
> ( (TextView) view.findViewById(android.R.id.summary)).setTextColor(mSummaryColor);
Flags: needinfo?(nalexander)
(In reply to Kalpesh Krishna from comment #57)
> Okay so what would I do to 
> > title = (TextView) view.findViewById(android.R.id.title);
> > summary = (TextView) view.findViewById(android.R.id.summary);
> > title.setTextColor(mTitleColor);
> > summary.setTextColor(mSummaryColor);
> 
> Do I need to setTextColor at all? or should I do it like this :- 
> > ( (TextView) view.findViewById(android.R.id.summary)).setTextColor(mSummaryColor);

Could do.  Up to you if you want an intermediate:

final TextView title = (TextView) ...
title.setTextColor(...)
Flags: needinfo?(nalexander)
I hope it is okay now @nalexander
Attachment #8675143 - Attachment is obsolete: true
Attachment #8681983 - Attachment is obsolete: true
Attachment #8682776 - Flags: review?(nalexander)
Flags: needinfo?(vivekb.balakrishnan)
Should I add the "checkin-needed"?
Flags: needinfo?(vivekb.balakrishnan)
No need. I landed this for you :)
Flags: needinfo?(vivekb.balakrishnan)
Marking this as resolved hence. :)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8682776 [details] [diff] [review]
colorfix.patch, removing member variables

Review of attachment 8682776 [details] [diff] [review]:
-----------------------------------------------------------------

vivek landed this, all good.
Attachment #8682776 - Flags: review?(nalexander)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.