Closed
Bug 1077755
Opened 10 years ago
Closed 10 years ago
Implement new tablet menu bar pressed/focused private browsing button color
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(3 files, 4 obsolete files)
(deleted),
image/png
|
antlam
:
feedback+
|
Details |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Followup because we need to make these items `Themed*` and it's a pain.
Assignee | ||
Comment 1•10 years ago
|
||
via bug 1070087:
(In reply to Anthony Lam (:antlam) from comment #30)
> Comment on attachment 8499908 [details]
> Private browsing, tapped button, smaller visual
>
> Can't find where I left this value but if it's not too much trouble could we
> do #222222 for V1 visual feedback on-press? For focused (with non-touch
> devices) we could use #363B40 for now (same as the background).
Blocks: new-tablet-v1
Comment 2•10 years ago
|
||
Good idea! I also found where I left that value, just a couple comments above in that same bug :P
Assignee | ||
Comment 3•10 years ago
|
||
Bug 1077195 will determine the final colors for private browsing.
Depends on: 1077195
Assignee | ||
Comment 4•10 years ago
|
||
I hate to complicate the generated code any more but oh well.
Attachment #8502899 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8502900 -
Flags: feedback?(alam)
Comment 6•10 years ago
|
||
Comment on attachment 8502899 [details] [diff] [review]
Change pressed/focused private browsing colors of new tablet menu items
Review of attachment 8502899 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. I'm just not convinced that STYLE_CONSTRUCTOR is necessary.
::: mobile/android/base/widget/ThemedView.java.frag
@@ +30,5 @@
> private boolean mIsDark;
> private boolean mAutoUpdateTheme = true;
>
> public Themed@VIEW_NAME_SUFFIX@(Context context, AttributeSet attrs) {
> +#ifndef STYLE_CONSTRUCTOR
I see no reason to make this constructor optional. Does it cause any build errors if you simply added it for all ThemedViews?
Attachment #8502899 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #6)
> I see no reason to make this constructor optional. Does it cause any build
> errors if you simply added it for all ThemedViews?
While I haven't tried it, LinearLayout's three param constructor requires API 11 [1] and one of the other Views (I don't remember off-hand) does not work either.
I could simplify it by always having the three argument constructor, but calling `super(arg1, arg2)` on some and `super(arg1, arg2, arg3)` on others – I'll do that, unless you disagree.
[1]: https://developer.android.com/reference/android/widget/LinearLayout.html#LinearLayout(android.content.Context, android.util.AttributeSet, int)
Comment 8•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > I see no reason to make this constructor optional. Does it cause any build
> > errors if you simply added it for all ThemedViews?
>
> While I haven't tried it, LinearLayout's three param constructor requires
> API 11 [1] and one of the other Views (I don't remember off-hand) does not
> work either.
>
> I could simplify it by always having the three argument constructor, but
> calling `super(arg1, arg2)` on some and `super(arg1, arg2, arg3)` on others
> – I'll do that, unless you disagree.
Works for me.
Assignee | ||
Updated•10 years ago
|
Attachment #8502899 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8503332 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Backed out in comment 11 for Fx-Team bustage (https://tbpl.mozilla.org/?tree=Fx-Team&rev=fa1f219e3eba).
bug 1075531 was also backed out, but I don't think that caused the issue.
Assignee | ||
Comment 14•10 years ago
|
||
It seems the issue is that we weren't calling super(arg1, arg2) for the two argument constructors, which defines a default style for each class such as "com.android.internal.R.attr.imageButtonStyle".
Attachment #8504135 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 15•10 years ago
|
||
Note that if we wanted to be extra safe, we could avoid making changes in unnecessary places (i.e. everywhere but ThemedImageButton).
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
Comment on attachment 8504135 [details] [diff] [review]
Part 2: Remove style constructor from unnecessary generated views
Review of attachment 8504135 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/widget/ThemedView.java.frag
@@ +41,5 @@
> a.recycle();
> }
>
> +#ifdef STYLE_CONSTRUCTOR
> + public Themed@VIEW_NAME_SUFFIX@(Context context, AttributeSet attrs, int defStyle) {
The code duplication in these constructors is a bit unfortunate. No good way to avoid it? I don't feel so strongly about this because this is just a template anyway.
Attachment #8504135 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Lucas Rocha (:lucasr) from comment #17)
> The code duplication in these constructors is a bit unfortunate. No good way
> to avoid it?
One way is to look up the default styles by hand and include them in each template file. However, this duplicates the original source code and code face problems with new versions of Android.
We could also remove the `final` modifiers on some of these variables and call out to a shared method, at the cost of that type safety.
I'd be down to implement the second if you prefer.
Comment 19•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #18)
> (In reply to Lucas Rocha (:lucasr) from comment #17)
> > The code duplication in these constructors is a bit unfortunate. No good way
> > to avoid it?
>
> One way is to look up the default styles by hand and include them in each
> template file. However, this duplicates the original source code and code
> face problems with new versions of Android.
>
> We could also remove the `final` modifiers on some of these variables and
> call out to a shared method, at the cost of that type safety.
>
> I'd be down to implement the second if you prefer.
Yeah, that sounds like a good compromise (removing the final modifiers) given that this is generated code that is unlikely to change often.
Assignee | ||
Comment 20•10 years ago
|
||
Called into initialize method, as recommended.
Assignee | ||
Updated•10 years ago
|
Attachment #8504135 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8505073 -
Flags: review+
Comment 21•10 years ago
|
||
Comment on attachment 8502900 [details]
Private browsing, tapped button
Looks very subtle but I'm ok with this as a first iteration (kinda like how it goes with the whole dark theme)...
We can refine as we go :) looks good!
Attachment #8502900 -
Flags: feedback?(alam) → feedback+
Assignee | ||
Comment 22•10 years ago
|
||
Rebase.
Assignee | ||
Updated•10 years ago
|
Attachment #8503332 -
Attachment is obsolete: true
Assignee | ||
Comment 23•10 years ago
|
||
Rebase.
Assignee | ||
Updated•10 years ago
|
Attachment #8505073 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8505829 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8505830 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Ignore comment 24 - bzexport fail.
Assignee | ||
Comment 26•10 years ago
|
||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43ae53dc6394
https://hg.mozilla.org/mozilla-central/rev/73da2cbb36a5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•4 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
•