better estimate of mm distances and dp/sp mesurement units support?

[Thomas]

I am currently developping a set of custom components that follow the material design guidelines (https://material.io/guidelines/). To this end, I needed to handle the dp (and sp for fonts) mesurement unit  (https://material.io/guidelines/layout/units-measurements.html#units-measurements-pixel-density). As dp are density independent, like milimeters (1mm ~= 6.299dp), I take a look at the way CN1 was handeling mm. I was a bit surprised by the fact that it uses density ranges (like the density in android DisplayMetrics: https://developer.android.com/reference/android/util/DisplayMetrics) to approximate millimeters distances whereas, on some platforms, most precise informations are available and could be used (like xdpi and ydpi on android for example). I think an improvment would be to use these more precise mesures, when available (and to default to the current way of converting mm to physical pixels when not), to compute mm distances. Also I think supporting the dp and sp units in layout constraints would be desirable as most android developpers (and beyond, as material design is becoming a standard for mobile GUIs ) are familiar with those.

What do you think?

[Shai Almog]

We do that because the precise information doesn't actually work.
That might have changed for newer devices by now so it might make sense to revisit that but back when we launched it turned out that Google didn't check these values for devices. So a lot of manufacturers just didn't update them when building new devices and we ended up with huge discrepancies. Notice that this was during the 2.x era so it might be a good idea to add some test logic that verifies the approximate values so this works universally and accurately.

[Thomas]

I started to make the changes into CN1 source code to improve how densities are handled. One of the only issue I have is that currently density constants are small int values that actually do not match the PPI (pixels per inch) density. It would make more sense to have them as PPI values (like android does). What is the reason of not having them directly as PPI values (there might be some not obvious good reason that I do not see)? Do you see any objection of having them changed to PPI values (it would also requiere to handle some special case for MultipleImages created before this change as, if I understood correctly, these density values are used to identify the matching image for the device resolution. So if a density coded into a multipleImage is <=80 it would have to be converted to PPI before testing (but  PPI values < 80 do not make sense on any device used nowadays, so this test should not be an issue)).

Appart from that, for iOS devices, I was thinking of having a hard coded conversion map between devices models and their PPI (the list iOS devices is short so it is something that is quite easy to maintain. If the device is not in the list it would default to the PPI assumption, like for android devices anyway, so it doesn't really matter if the list of devices is not up to date). For Android, I was thinking of using xdpi and ydpi and see if the PPI obtain with these values do not deviate too much from the estimated PPI (based on device resolution), if not, I keep that value that should be more precise than the estimated one, else I default to the estimate (as xdpi and ydpi might be erroneous)

For other platforms, I don't know what whould be the best approach (on windows desktops, the ppi is a system variable that is 96 by default and can be changed by the user, so maybe taking this value if not at 96 could be an idea...?) so I would befault to the PPI assumption based on resolution for now...

Sounds good to you? If yes I would probably send a pull request with these changes in the comming days.

[Shai Almog]

The reason we work with millimeters is mostly for uniformity across platforms as a universal standard. Changing to the Android DIP standard would be problematic as all the tooling/theming etc is based around millimeters so we can expose that in an API but not leverage this. It wouldn't help.

Are you talking about this method? :

    @Override
    public int getDeviceDensity() {
        DisplayMetrics metrics = new DisplayMetrics();
        if (getActivity() != null) {
            getActivity().getWindowManager().getDefaultDisplay().getMetrics(metrics);
        } else {
            metrics = getContext().getResources().getDisplayMetrics();
        }

        if(metrics.densityDpi < DisplayMetrics.DENSITY_MEDIUM) {
            return Display.DENSITY_LOW;
        }

        if(metrics.densityDpi < 213) {
            return Display.DENSITY_MEDIUM;
        }

        // 213 == TV
        if(metrics.densityDpi >= 213 &&  metrics.densityDpi <= DisplayMetrics.DENSITY_HIGH) {
            return Display.DENSITY_HIGH;
        }

        if(metrics.densityDpi > DisplayMetrics.DENSITY_HIGH && metrics.densityDpi < 400) {
            return Display.DENSITY_VERY_HIGH;
        }

        if(metrics.densityDpi >= 400 && metrics.densityDpi < 560) {
            return Display.DENSITY_HD;
        }

        if(metrics.densityDpi >= 560 && metrics.densityDpi <= 640) {
            return Display.DENSITY_2HD;
        }
        if(metrics.densityDpi > 640) {
            return Display.DENSITY_4K;
        }

        return Display.DENSITY_MEDIUM;
    }

In this case using the densityDpi field makes a lot of sense as it's the officially sanctioned Android API that does exactly this.

I'm talking about this method only:

    public int convertToPixels(int dipCount, boolean horizontal) {
        DisplayMetrics dm = getContext().getResources().getDisplayMetrics();
        float ppi = dm.density * 160f;
        return (int) (((float) dipCount) / 25.4f * ppi);
    }

Right now this is defensive code designed against the bugs in dm.xdpi/ydpi which I'm assuming you want to use instead. To work correctly you would need to guard against misconfigured xdpi by checking that the range difference isn't too big.

Notice that this method is usually invoked with *1000 of the value asked and then converted to a floating point value for higher accuracy.

[Thomas]

Yes I want to keep the millimeter unit as a supported mesurement unit in CN1, don't worry. But I also want to add dp and sp as possible additional units. In any case, I think it makes more sense to make getDeviceDensity() directly return the PPI value (so the value in metrics.densityDpi in Android for example or a more exact measure based on xdpi and ydpi. So rather than having a value of 30 for DENSITY_MEDIUM devices, the density value would be 160). I made the changes into the default CodenameOneImplementation class (the one into CodenameOne and all related classes (the Display and MultipleImages related ones)), now I will do it into the CodenameOneImplementation of each platform port that override the modified functions.

That way, the convertToPixels function can just be wrote like this (Remark: getDevicePPI() function is the getDeviceDensity() new function that return the density in PPI (I renamed it to be sure not to miss any dependency into my changes):

 public int convertToPixels(int dipCount, boolean horizontal) { 

        return convertMillimetersToPixels(dipCount, horizontal);

    }

    

    public int convertMillimetersToPixels(double millimeters, boolean horizontal) { 

    return (int) (getDevicePPI()/25.4*millimeters); //TODO: use Math.round() rather than int casting? Would be more precise but affect performances a bit as int casting is probably quicker than rounding 

[Shai Almog]

You can't make that change because it will break binary compatibility. Constants in Java are linked into the class file so if you change their value you will break the compilation.

[Thomas]

Sorry but I don't understand what ou are saying here. I probably did not explain clearly enaugh the changes I am proposing. I think submiting a pull request would be easier than trying to explain in detail (I would do it as soon as my previous pull request is reviewed). Anyway I compilled with my changes for JavaSE and Android and did not see any issue so far. However, I have a few questions:

- in the JavaSEPort, I don't really understand the relation between the DefaultPixelMilliRatio, the ScreenResolution() and the retinaScale. Do tk.getScreenResolution() not return the screen resolution in dots per inch? if yes, why is it multiplied by the retinaScale to be transformed into DefaultPixelMilliRatio? is this retinaScale really needed? If I look at the IOSImplementation for example, the density for retina devices is directly computed into DIPs and there is no further need to know if a device is a retina one or not. 

- In IOSImplementation, is it possible to access the systemInfo.machine platform variable? If so, how? This would allow to better estimate the screen density that relying on rules on the screen resolution (that do not allow to identify iPad Mini gen 2 to 4 from other iPads): https://stackoverflow.com/questions/11197509/how-to-get-device-make-and-model-on-ios

[Shai Almog]

I suggest creating a branch and submitting a second pull request. As I mentioned in the PR the table layout change is very risky and would need a serious review process.

Unfortunately we don't have enough test coverage for table layout behaviors. If we did we could skim the review a bit.

* The retina in the JavaSE port refers to retina Mac desktops (MacBook pro and MacBook). By default a retina device uses pixel duplication for compatibility so there are a few hacks there to make these machines expose their full density.

* You can access that in the native code of the port. I think it's a problematic path as device detection quickly becomes a bottomless pit. BTW Diamond recently contributed this cn1lib which you might find interesting: https://github.com/diamonddevgroup/CN1-Device/