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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Jack Linhart]

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?

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

Code is at https://github.com/mozilla-services/android-sync/ which is inserted into the Mozilla HG repository at https://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/sync

Comment 3

[Jack Linhart]

(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.

Comment 4

[Richard Newman [: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

Comment 5

[Filip Dyrčík]

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.

Comment 6

[Richard Newman [:rnewman]]

Go right ahead, Filip.

Comment 7

[Nicholas Rosbrook [:enr0n]]

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?

Comment 8

[Richard Newman [: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.

Comment 9

[Nicholas Rosbrook [:enr0n]]

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?

Comment 10

[Richard Newman [: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.

Comment 11

[Nicholas Rosbrook [:enr0n]]

Attachment: partial patch

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.

Comment 12

[Richard Newman [: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.

Comment 13

[Richard Newman [:rnewman]]

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.

Comment 14

[Nicholas Rosbrook [:enr0n]]

Attachment: patch

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 8635056 [details] [diff] [review]
patch

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

Looks good!

Comment 16

[Richard Newman [:rnewman]]

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.

Comment 17

[Nicholas Rosbrook [:enr0n]]

Attachment: patch_2

My mistake -- I fixed them.

Comment 18

[Richard Newman [:rnewman]]

url:        https://hg.mozilla.org/integration/fx-team/rev/2608b66c234580d4f084e51f3bc361a3c1d4c74e
changeset:  2608b66c234580d4f084e51f3bc361a3c1d4c74e
user:       Nick Rosbrook <[e-mail]>
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

Comment 19

[Richard Newman [:rnewman]]

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>!

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2608b66c2345