Closed
Bug 912523
Opened 11 years ago
Closed 11 years ago
Give bookmark thumbnails a context menu header
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: pretzer, Assigned: capella)
References
Details
(Whiteboard: [mentor=sriram][lang=java])
Attachments
(4 files)
59.89 KB,
image/png
|
Details | |
2.53 KB,
patch
|
sriram
:
review+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
sriram
:
review-
|
Details | Diff | Splinter Review |
4.94 KB,
patch
|
Details | Diff | Splinter Review |
The six bookmark thumbnails on about:home deserve a header for their context menu showing the title/URL of the bookmark, just like all the normal bookmarks below.
Reporter | ||
Comment 1•11 years ago
|
||
Updated•11 years ago
|
Blocks: new-about-home
Comment 4•11 years ago
|
||
(In reply to Sriram Ramasubramanian [:sriram] from comment #3) > I can fix it right away. But happy to mentor :) Let's make this a mentor bug, but you can fix it if you run out of things to do and nobody else fixes it first :)
Whiteboard: [mentor=sriram][lang=java]
Assignee | ||
Comment 5•11 years ago
|
||
I'll snag this small one while you guys are working on polishing the big about:home finishing touches :D
Comment 6•11 years ago
|
||
Comment on attachment 799955 [details] [diff] [review] bug912523 Review of attachment 799955 [details] [diff] [review]: ----------------------------------------------------------------- ++ r ++ ::: mobile/android/base/home/BookmarksPage.java @@ +286,5 @@ > MenuInflater inflater = new MenuInflater(view.getContext()); > inflater.inflate(R.menu.top_bookmarks_contextmenu, menu); > > TopBookmarksContextMenuInfo info = (TopBookmarksContextMenuInfo) menuInfo; > No new line needed before this.
Attachment #799955 -
Flags: review?(sriram) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Nit fixed, try pushed: https://tbpl.mozilla.org/?tree=Try&rev=27bbff180d1c
Comment 8•11 years ago
|
||
Comment on attachment 799955 [details] [diff] [review] bug912523 Review of attachment 799955 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/home/TopBookmarksView.java @@ +243,5 @@ > } > + > + public String getDisplayTitle() { > + return TextUtils.isEmpty(title) ? url.replaceAll(REGEX_URL_TO_TITLE, "") : title; > + } Drive by. This will be the third implementation of stripping the scheme/subdomain from urls that we have in our Java code. Please use the ones in StringUtils (and if you want, combine those two so that there's only one implementation of this).
Assignee | ||
Comment 9•11 years ago
|
||
Something like this might be better?
Attachment #800020 -
Flags: review?(sriram)
Assignee | ||
Comment 10•11 years ago
|
||
Or we could go further like this? I wasn't sure by wesj comments if he thought we could combine both stripScheme() and stripCommonSubdomains() into one method (versus the two) in StringUtils.java ... there's a unique call to stripCommonSubdomains() here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#335
Reporter | ||
Comment 11•11 years ago
|
||
Stripping common subdomains for bookmarks is not what we want I think. There are places where it makes sense (like in the addressbar or reader mode) because either space is very limited or it doesn't add any useful information in that particular context. Both is not the case here though, in my eyes. Therefore calling stripScheme() should be sufficient here. Offering a combined method for consumers that do both anyways does probably make sense. What I *think* Wes meant though was that there are still other places in the code that implement their own stripping mechanism and you could convert them to the StringUtils methods while your at it if you want. Just my two cents!
Reporter | ||
Comment 12•11 years ago
|
||
Actually after thinking about it again, it really doesn't matter much if the common subdomains get stripped away here. Patch v3 is therefore probably the way to go, I think. Sorry if I caused any confusion.
Comment 13•11 years ago
|
||
Comment on attachment 800020 [details] [diff] [review] bug912523 v2 Review of attachment 800020 [details] [diff] [review]: ----------------------------------------------------------------- v1 is better to me.
Attachment #800020 -
Flags: review?(sriram) → review-
Assignee | ||
Comment 14•11 years ago
|
||
shipping v1 ... planning a followup patch re: sriram and wesj via irc https://hg.mozilla.org/integration/fx-team/rev/201d6846b308
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/201d6846b308
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
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
•