Closed Bug 1005231 Opened 6 years ago Closed 4 years ago

Show match count in Find toolbar

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1077574

People

(Reporter: wesj, Assigned: adityamagadi, Mentored)

References

(Depends on 1 open bug)

Details

(Whiteboard: [lang=js][lang=java])

Attachments

(1 file)

Desktop just landed the ability to count found items (bug 257061). We should try to get some UI for this in Fennec too.

There's also the ability to know "where" in the document the mathces are now too, so we could theoretically show them in the scrollbar. Separate bug though :)
wesj: this looks like a great mentored bug, if we got some UI guidance.  If you agree, can you update the flags and add some additional direction?
This isn't a trivial bug to fix, but I think it would be a fun project for someone to work on.

You'll want to look in these files for code related to the find bar:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/FindHelper.js
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/FindInPageBar.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/find_in_page_content.xml

The first part of this will involve updating the native UI to hold the match count somewhere (we'll want to get feedback from our UX team on that), and the second part will involve updating FindHelper.js to use the Finder.jsm API to get the number of matches. You can look at this desktop code for inspiration:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml#462
Whiteboard: [mentor=margaret][lang=javascript][lang=java]
Hi Wesley,
I would like to work on this bug, however am absolute beginner to mozilla can you please help me get started on this one?
Assignee: nobody → adityamagadi
Status: NEW → ASSIGNED
Hi Aaron,
Thank you for assigning me on this bug. Can you please provide some information on how to get started on this one?
Flags: needinfo?(margaret.leibovic)
(In reply to adityamagadi from comment #4)
> Hi Aaron,
> Thank you for assigning me on this bug. Can you please provide some
> information on how to get started on this one?

First of all, do you have a development environment set up? If not you should follow the directions here:
https://wiki.mozilla.org/Mobile/Fennec/Android

After that, you should poke around in the files I mentioned in comment 2, and try making some changes to see if you can get the match count to show up in the find bar. 

Once you get the basic plumbing in place, we can talk to our UX team about what we should make this look like. I'm a bit concerned that we don't have very much space in the find bar, so we may need to be creative about how we display this information.
Flags: needinfo?(margaret.leibovic)
okay, thank you :)
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=javascript][lang=java] → [lang=javascript][lang=java]
Depends on: 923370
Hi there, I have recently joined this community, now starting(#1) with mentored bugs, Can somebody please provide info on this bug and other essential docs helpful for me(beginner).
Hey, Ankur. I noticed that you're already assigned to two other bugs - I think you should only focus on one or two at a time to prevent overloading yourself (particularly as a new contributor!). Let us know which ones you want to work on.
Whiteboard: [lang=javascript][lang=java] → [lang=js][lang=java]
I've started working on this -- the plumbing is more or less done I think (for now I've just hardcoded a TextView into the find bar to show the numbers in the UI -- i.e. purely for testing) -- so I'm guessing this would be the right time to ask for UX advice?

(With this patch applied the Find bar feels a bit crowded on the emulator (although it's actually fine on a Galaxy Note...)).

I've attached a preview patch just in case anyone is interested in seeing the current state of things.
Comment on attachment 8496545 [details] [diff] [review]
Prototype / plumbing for find count.

Great start!  This all looks very reasonable to me, but I don't think I'm confident enough to review it.
Attachment #8496545 - Flags: feedback+
ah, just found this, nalexander

Seems dup / collision with Bug 1077574
capella: I see andrezj's patch has not been reviewed.  The ticket you've been pushing on is definitely a duplicate of this one.  Can you own reviewing and integrating andrezj's patch with your patch?  I don't want andrezj to be ignored :)
Flags: needinfo?(michael.l.comella)
(In reply to Nick Alexander :nalexander from comment #12)
> capella: I see andrezj's patch has not been reviewed.  The ticket you've
> been pushing on is definitely a duplicate of this one.  Can you own
> reviewing and integrating andrezj's patch with your patch?  I don't want
> andrezj to be ignored :)

I think capella is also working on some further things with Promise/resolver stuff in his patch, so it might be easiest just to abandon my patch (I think our initial approaches were very similar anyways)? Not a problem at all for me, just a bit of unlucky timing I guess :).

(I probably won't have much time except very occasionally at weekends in the near future, so perhaps better from that perspective too.)
Flags: needinfo?(michael.l.comella) → needinfo?(markcapella)
Sorry, that was intended to be capella.
Hey andrezj,

I really hate that we collided here, especially since we work hard to encourage new contributors :(

I'd started from almost the exact same place as you did, so your patch is almost entirely incorporated into the final work as being promoted through bug 1077574 ...

You might like to read through there(?) ... we actually upgraded the messaging logic to use the new Java-JS sync calls which simplified things. We also realized that we didn't need to use the new Javascript Promise capabilites as the .requestMatchesCount() call to Finder.jsm wasn't async, so the results were available immediately via the "listener".

The UI on a phone is a little crowded as you noted, but I think we'll get by. fyi? Take a look at the follow-on bug 1050480 where we're adding a |match case| button ... :D

phone: https://www.dropbox.com/s/v116an02cgif2xd/bug1050480_matchCaseOff.png?dl=0
tablet (nicer): https://www.dropbox.com/s/z9w5uxyudubaqcn/bug1050480_matchCaseOn.png?dl=0

I do a lot of work on the weekends also, so if you have time as you mention above grab another project! I'm usually around on #mobile and/or #introduction and will do what I can to help :-)
Flags: needinfo?(markcapella)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1077574
You need to log in before you can comment on or make changes to this bug.