Use the native New Tab by default - #1830
Conversation
02de6b4 to
12f76b6
Compare
| } else if (checkedId == CHOICE_HOMEPAGE_URL) { | ||
| setHomepageValue(mDefaultHomepageUrl); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need a fallback in case none of the conditional clauses is satisfied ?
There was a problem hiding this comment.
I don't think so, the value comes from a group of three radio buttons so it can only take the values 0, 1, and 2.
There was a problem hiding this comment.
Then perhaps it'd be clearer to use a switch and an assert to ensure no mismatch is possible.
|
Umm, there are a few regressions in this change: 1- The first time you select "other", the about:newTab appears as hint. If clicked save, the next time it appears as wolic.com choice. |
|
The implementation of this UI is already buggy upstream. For example, it lets you edit the description of the homepage instead of the actual URL:
|
|
In this PR, I tried to make things more predictable with a small number of changes. The idea was that the "other" value would default to the current value. Case 1:
Alternatively, we could make the edit field always use the homepage URL by default. Like this:
Would that be better? |
Clean up the code and fix some edge cases where the homepage was being incorreclty updated. Use the current homepage value for the URL edit field, because otherwise it was far too confusing. Do not assume that our home webpage will be the default (for example, the default might be to show the native New Tab UI).
12f76b6 to
a635a16
Compare
|
I'll let @javifernandez review this as he was the one detecting issues |
|
Umm, what you mean is that the "other" option was already buggy before this PR and that it hasn't been never possible to set a custom home page ? In any case, I think I understand now what you tried to implement; when selected other, if the user didn't change the value of the text-input field, Wolvic doesn't change the new selected value in the radio-button. You know more about usability than me, but I find it a bit confusing. In my mind, other means whatever it's specified in the text input field, either it it remains with the default value or not. Alternatively, we can use a different default value, kind of "about:blank", so that the user is forced to set one valid value there. |
javifernandez
left a comment
javifernandez
left a comment
There was a problem hiding this comment.
I suggested some changes, but perhaps not a blocker to land this patch and continue the discussion offline.
|
I think there are issues related to the new tab page that should be fixed before landing this. |
3 participants
Use the native New Tab by default. Also clean up some bugs when setting the homepage in the Settings.