Closed
Bug 964508
Opened 10 years ago
Closed 10 years ago
Rename TwoLineRow members to match the latest dataset terminology
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(1 file, 2 obsolete files)
15.41 KB,
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
PrimaryText -> Title SecondaryText -> Description
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8367489 -
Flags: review?(margaret.leibovic)
Comment 2•10 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•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8367489 -
Attachment is obsolete: true
Assignee | ||
Comment 4•10 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•10 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•10 years ago
|
||
Assignee | ||
Comment 7•10 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•10 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•10 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•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4c412ebc25b7
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4c412ebc25b7
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•