Closed Bug 1122977 Opened 5 years ago Closed 4 years ago

Increase number of inline devices from 2 to N, based on screen sizes

Categories

(Firefox for Android :: Overlays, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: rnewman, Assigned: enr0n, Mentored)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(1 file, 2 obsolete files)

If you have fewer than 3 other devices connected to your Sync account, we'll show them in-line in the share overlay list.

This number is pretty arbitrary. Definitely on modern 4"+ devices, the overlay has enough room for more than one more. So let's raise the limit.

Bonus points: make the limit a resource, so that we can use different limits for different screen sizes simply by putting different XML files in resource directories.
This is my first attempt at a bug fix. I'd like to take a whack at this one but can't find the source code. How do I view it?
Assignee: nobody → iatenine
(In reply to Richard Newman [:rnewman] from comment #0)
> If you have fewer than 3 other devices connected to your Sync account, we'll
> show them in-line in the share overlay list.
> 
> This number is pretty arbitrary. Definitely on modern 4"+ devices, the
> overlay has enough room for more than one more. So let's raise the limit.
> 
> Bonus points: make the limit a resource, so that we can use different limits
> for different screen sizes simply by putting different XML files in resource
> directories.

OK, I see the code on Github and have been combing through the /src/ files, trying to use every hint at my disposal to find it with the search functions and have even tried some of the less sensible folders but I don't have time to dig and maintain my day job. Can you at least give me the file path of what I need to fix?

Searching with =2, = 2 or 2 all fail to come up with anything.
Flags: needinfo?(rnewman)
Sorry, Kevin gave you bad advice.

This code is in mozilla-central, so the main build instructions apply:

https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec


Once you have a build working, you can take a look at the source, which should be here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/overlays/ui/SendTabList.java#51


This bug is about removing that constant and instead reading it from an Android resource.

See here:

http://developer.android.com/guide/topics/resources/more-resources.html#Integer
Flags: needinfo?(rnewman)
Hello.

I´m entering the world of open source and I choosed this bug as a starter. I´m following steps in https://developer.mozilla.org/en-US/docs/Introduction document and preparing my development enviroment for building Fennec.

I would really like to work on this bug. I think initial information about the problem and proposed solution is clear to me.

Is it OK to start working on it despite the fact that bug has been already assigned? Jack looks like to be inactive.

Thank you for reply and other guidance in the future.
Flags: needinfo?(rnewman)
Go right ahead, Filip.
Assignee: iatenine → nobody
Flags: needinfo?(rnewman)
Richard,

For this do you just mean changing the 2 to something like R.integer.<some_number> ? Or do you want a value assigned after checking the screen size?
Flags: needinfo?(rnewman)
A bit of both: the idea is yes, to use R.integer.inline_share_device_count… and that value is determined by the Android resource system. We'd include different numeric values for different device categories, so mdpi devices would get 2, xxxhdpi devices perhaps 4, and so on.
Flags: needinfo?(rnewman)
Okay for a check I was thinking something like this: 
switch(getresources().getDisplayMetrics()
       .densityDpi) {
    case DisplayMetrics.DENSITY_MEDIUM:
         MAXIMUM_INLINE_ELEMENT = 
         R.integer.inline_device_mdpi;
         break;

 // so on for others
}

Any advice? Is there a better way to do this?
Flags: needinfo?(rnewman)
Yes, as I mentioned: use the Android resource system.

http://developer.android.com/guide/topics/resources/more-resources.html#Integer

At the risk of actually writing the patch: add to m/a/b/resources/values/integers.xml:

  <integer name="number_of_inline_share_devices">2</integer>

and add a similar file to m/a/b/resources/values-{large,xlarge}-v11/integers.xml with larger numbers -- perhaps 3 and 4 respectively.

Then just use R.integer.number_of_inline_share_devices instead of 2.
Flags: needinfo?(rnewman)
Attached patch partial patch (obsolete) — Splinter Review
This gets at values in m/a/b/resources/values/integers.xml as well as /values-xlarge-v11/integers.xml...

However, there is no /values-large-v11/integers.xml -- I tried to add one but it doesn't show up in the patch.
Attachment #8634553 - Flags: review?(rnewman)
(In reply to Nicholas Rosbrook [:enr0n] from comment #11)

> However, there is no /values-large-v11/integers.xml -- I tried to add one
> but it doesn't show up in the patch.

You need to `hg add` it.
Assignee: nobody → nrosbrook
Status: NEW → ASSIGNED
Comment on attachment 8634553 [details] [diff] [review]
partial patch

Review of attachment 8634553 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/values/integers.xml
@@ +13,1 @@
>    

Please fix this trailing whitespace while you're here.
Attachment #8634553 - Flags: review?(rnewman) → review+
Attached patch patch (obsolete) — Splinter Review
Attachment #8634553 - Attachment is obsolete: true
Attachment #8635056 - Flags: review?(rnewman)
Comment on attachment 8635056 [details] [diff] [review]
patch

Review of attachment 8635056 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8635056 - Flags: review?(rnewman) → review+
Comment on attachment 8635056 [details] [diff] [review]
patch

Review of attachment 8635056 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/values-xlarge-v11/integers.xml
@@ +7,5 @@
>  
>      <integer name="number_of_top_sites">9</integer>
>      <integer name="number_of_top_sites_cols">3</integer>
>      <integer name="panel_icon_grid_view_columns">6</integer>
> +    <integer name="number_of_inline_share_devices>4</integer>

More accurately, it looks good with the missing quotes added :)

I'll test and land this.
Attached patch patch_2Splinter Review
My mistake -- I fixed them.
Attachment #8635056 - Attachment is obsolete: true
Attachment #8637227 - Flags: review?(rnewman)
url:        https://hg.mozilla.org/integration/fx-team/rev/2608b66c234580d4f084e51f3bc361a3c1d4c74e
changeset:  2608b66c234580d4f084e51f3bc361a3c1d4c74e
user:       Nick Rosbrook <thisisnickrosbrook@gmail.com>
date:       Thu Jul 16 20:53:53 2015 -0400
description:
Bug 1122977 - Make the share overlay's MAXIMUM_INLINE_ELEMENTS fetch its value from resources, instead of being fixed at 2. r=rnewman
Attachment #8637227 - Flags: review?(rnewman) → review+
Nicholas: do you have any idea about what you want to work on next? Come find nalexander, liuche, or myself on IRC, or take a look at <http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&java=1&unowned=1>!
https://hg.mozilla.org/mozilla-central/rev/2608b66c2345
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.