Rename TwoLineRow members to match the latest dataset terminology

RESOLVED FIXED in Firefox 30

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lucasr, Assigned: lucasr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 30
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
PrimaryText -> Title
SecondaryText -> Description
(Assignee)

Comment 1

4 years ago
Created attachment 8367489 [details] [diff] [review]
Rename TwoLineRow members to match the current dataset terminology (r=margaret)
(Assignee)

Updated

4 years ago
Attachment #8367489 - Flags: review?(margaret.leibovic)

Comment 2

4 years ago
Comment on attachment 8367489 [details] [diff] [review]
Rename TwoLineRow members to match the current dataset terminology (r=margaret)

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

This patch looks fine, but I want us to give the icons better names before we land it.

::: mobile/android/base/home/TwoLineRow.java
@@ +20,5 @@
>  public abstract class TwoLineRow extends LinearLayout {
>      protected static final int NO_ICON = 0;
>  
> +    private final TextView mTitle;
> +    private int mTitleIconId;

Oh, weird, I thought that the primary icon was the big favicon icon, but looking at this code more closely, I see that it's for the small icon that appears on the right side of the description. So the fact that we originally called these primary/secondary icons is confusing, and I don't think we should call them title/description icons either, since they actually both appear on the description row. Maybe we should call them leftIcon and rightIcon? Or leftDescriptionIcon and rightDescriptionIcon?
Attachment #8367489 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 3

4 years ago
Created attachment 8371375 [details] [diff] [review]
Rename TwoLineRow members to match the current dataset terminology (r=margaret)
(Assignee)

Updated

4 years ago
Attachment #8367489 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Comment on attachment 8371375 [details] [diff] [review]
Rename TwoLineRow members to match the current dataset terminology (r=margaret)

primaryText -> title
secondaryText -> description
primaryIcon -> rightDescriptionIcon
secondaryIcon -> leftDescriptionIcon
Attachment #8371375 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 5

4 years ago
Comment on attachment 8371375 [details] [diff] [review]
Rename TwoLineRow members to match the current dataset terminology (r=margaret)

Scratch that, I actually think it makes more sense to move the icon bits to TwoLinePageRow.
Attachment #8371375 - Attachment is obsolete: true
Attachment #8371375 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 6

4 years ago
Created attachment 8371473 [details] [diff] [review]
Rename TwoLineRow members and move icon code to TwoLinePageRow (r=margaret)
(Assignee)

Comment 7

4 years ago
Comment on attachment 8371473 [details] [diff] [review]
Rename TwoLineRow members and move icon code to TwoLinePageRow (r=margaret)

I'm a bit happier with this approach:
primaryText -> title
secondaryText -> description
primaryIcon -> pageTypeIcon (in TwoLinePageRow)
secondaryIcon -> switchToTabIcon (in TwoLinePageRow)

This way we can use more meaningful names for the description icons as they are only used in TwoLinePageRow.
Attachment #8371473 - Flags: review?(margaret.leibovic)

Comment 8

4 years ago
Comment on attachment 8371473 [details] [diff] [review]
Rename TwoLineRow members and move icon code to TwoLinePageRow (r=margaret)

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

Nice, this is much clearer.

::: mobile/android/base/home/PanelListRow.java
@@ +55,3 @@
>  
>          int urlIndex = cursor.getColumnIndexOrThrow(URLColumns.URL);
>          final String url = cursor.getString(urlIndex);

Is there a bug filed to have this get the description now that we have that in the schema? If not, we can probably just do that here, since it would be a small change.

I was actually thrown off with my test add-on, since the URL showed up, not the description I set.

::: mobile/android/base/home/TwoLineRow.java
@@ +17,5 @@
>  
>  import java.lang.ref.WeakReference;
>  
>  public abstract class TwoLineRow extends LinearLayout {
>      protected static final int NO_ICON = 0;

NO_ICON is only used in TwoLinePageRow now, we should probably move it there, unless we have other plans for icons in TwoLineRow.
Attachment #8371473 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 9

4 years ago
(In reply to :Margaret Leibovic from comment #8)
> Comment on attachment 8371473 [details] [diff] [review]
> Rename TwoLineRow members and move icon code to TwoLinePageRow (r=margaret)
> 
> Review of attachment 8371473 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, this is much clearer.
> 
> ::: mobile/android/base/home/PanelListRow.java
> @@ +55,3 @@
> >  
> >          int urlIndex = cursor.getColumnIndexOrThrow(URLColumns.URL);
> >          final String url = cursor.getString(urlIndex);
> 
> Is there a bug filed to have this get the description now that we have that
> in the schema? If not, we can probably just do that here, since it would be
> a small change.

Filed bug 969316 and posted a separate patch there. 

> I was actually thrown off with my test add-on, since the URL showed up, not
> the description I set.
> 
> ::: mobile/android/base/home/TwoLineRow.java
> @@ +17,5 @@
> >  
> >  import java.lang.ref.WeakReference;
> >  
> >  public abstract class TwoLineRow extends LinearLayout {
> >      protected static final int NO_ICON = 0;
> 
> NO_ICON is only used in TwoLinePageRow now, we should probably move it
> there, unless we have other plans for icons in TwoLineRow.

Nice catch. Moved it to TwoLinePageRow.
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/4c412ebc25b7
https://hg.mozilla.org/mozilla-central/rev/4c412ebc25b7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.