gluonhq / substrate

Create native Java(FX) apps for desktop, mobile and embedded
GNU General Public License v2.0
390 stars 52 forks source link

Change -Dmonocle.input.touchRadius value to 2 #1183

Open nlisker opened 1 year ago

nlisker commented 1 year ago

Following https://stackoverflow.com/questions/74097583/combobox-makes-selection-with-touch-events-difficult, a value of 1 causes touch events to not register correctly on some devices (tested on 2 Samsung Galaxys). A value of 2 works better. I suggest changing the default for Andoird to 2 at https://github.com/gluonhq/substrate/blob/master/src/main/resources/native/android/c/launcher.c#L69.

johanvos commented 1 year ago

Good remark. I think moving it from 1 to 2 does not cause harm, but there might be a more generic solution. We changed the default a number of times before, since it used to be way too large for some devices, and then too small for others. I wonder if we should dynamically adapt it to the DPI property? The issue with that is that not all devices properly report DPI.

nlisker commented 1 year ago

If it's possible to adapt the value per device it would be best of course. Right now, a value of 1 in some devices makes some controls (ComboBox, ListView) practically unusable for item selection because the gesture is taken as a scroll event.

jperedadnr commented 1 year ago

My Pixel XL, where I don't have selection issues with input touch radius 1, reports (right after start):

GraalActivity: metrics = DisplayMetrics{density=3.5, width=1440, height=2392, scaledDensity=3.5, xdpi=537.882, ydpi=532.983}, density = 3.5

@nlisker Could you to get and post those metrics from your Samsung Galaxy devices?

nlisker commented 1 year ago

This is for the S7:

metrics = DisplayMetrics{density=3.0, width=1080, height=1920, scaledDensity=3.0, xdpi=435.42825, ydpi=431.57477}, density = 3.0
nlisker commented 1 year ago

The other device belongs to a colleague. It can take some time to obtain the info.

javateer commented 1 year ago

I also reported on this issue and setting -Dmonocle.input.touchRadius to a higher value does work but at a trade-off that scrolling is compromised. Widgets seem to move in a choppy fashion instead of gliding on the screen.

I found a software solution (encapsulated in one Java class I call GluonListViewSelectionAdapter) whereby touch is just as sensitive on a Samsung Galaxy phone as a Google Pixel, and it has nothing to do with changing -Dmonocle.input.touchRadius. My custom code looks kludge but, regardless, it demonstrates how to fix the problem. I can provide a HelloWorld app that applies my fix so that the Gluon Developer Team can see for themselves that all devices behave the same way to the users' touch events.

My hope is that, after seeing how I fixed it as an application developer, the platform developers will have new insight what they need to do so that every next application developer like myself doesn't also have to repeat the same workaround code that I've applied to my apps.

jperedadnr commented 1 year ago

@javateer Could you provide that HelloWorld class with your fix? That would be really helpful.

javateer commented 1 year ago

@jperedadnr @nlisker please pull my demo app from https://github.com/javateer/gluonfx-list-selection-adapter

The app shows you two lists of the same data. One of the lists is denoted as Default and the other as Adapted. The Default ListView behaves as usual. The Adapted ListView makes use of my GluonListViewSelectionAdapter.java.

If you run this on a Google Pixel or another non-Samsung device, both lists seem to behave the same to touch and scroll events. What's important is to deploy this app on a Samsung device. Then, you see that GluonFX ListView touch-selection events are very problematic for the Default ListView. They sometimes register but you usually don't get the expected/desired behavior. However, observing the Adapted ListView's touch events shows list item selection/deselection actions to be working consistently/reliably.

Even though every runtime environment does not require my GluonListViewSelectionAdapter.java for their apps' ListView touch events to work (only Samsung devices need this fix), a developer can apply this fix and then their app will work across all environments. Note though, I say this not having tested on iOS yet. Just my Linux desktop, my Samsung phone, and my Google Pixel phone.

nlisker commented 1 year ago

Tested on Galaxy S7 and I can confirm that the adapted list works with both selection and scroll events, and the default list works only with scroll events. How do you suggest to proceed with the fix?

I also get [X] symbols instead of icons. Screenshot_20230219-170323 Do you not get these?

javateer commented 1 year ago

@nlisker, regarding the Xed boxes you're seeing instead of the Material Design Icons - during my own development, as I made use of more of the Gluon API's surface area, I would come across another issue not exposed in the sample applications. In the forums I would find the solution (usually given by @jperedadnr) was to increment to a newer dependency release that patched the issue. However, that upgrade sometimes broke another feature that was working in the older revision. Then, as a follow-up to the reported introduction of a new bug by the code patch of repairing the earlier one, I would find a newer release to upgrade to.

While I don't now still remember precisely the incremental updates I've made (and their reasons with available StackOverflow links), I can tell you that I do remember seeing my working Icons break after an upgrade, and then restored after another upgrade.

This demo app I've provided (and you've test run) didn't show the Material Design Icons for me at one point, but it does now. My current build environment is:

$GRAALVM_HOME=/opt/graalvm/graalvm-svm-java17-linux-gluon-22.1.0.1-Final

You can see in my demo app's POM file the following configuration values. But you're running the same demo app so I expect you haven't changed these settings in your copy of the codebase's POM file.

<maven.compiler.release>11</maven.compiler.release>
<javafx.version>18.0.1</javafx.version>
<javafx.plugin.version>0.0.8</javafx.plugin.version>
<gluonfx.plugin.version>1.0.12</gluonfx.plugin.version>
<charm.version>6.2.2</charm.version>
<attach.version>4.0.15</attach.version>

Consider also deleting directory ~/.gluon/Android (or at least renaming to something else like ~/.gluon/_Android), then rebuild the project, allowing that directory and its contents to restore itself. I believe I may have had some sort of caching issue that was cleared out when I did that.

Screenshot from 2023-03-13 18-30-05

javateer commented 1 year ago

Sorry, @nlisker. My screenshot above was taken from the Desktop. I just deployed this to my Google Pixel 6 and I see boxes too. I suppose this is another regression bug.

Are you still working on Gluon @johanvos? Your social media posts suggest you've stayed busy in other things. Your genius skills and attention here would be appreciated :)

johanvos commented 1 year ago

Hi @javateer , I'm totally still with Gluon and working on Substrate :) I think this issue is in the great hands of @jperedadnr and we'll both have a look at the question.

jperedadnr commented 1 year ago

Typically "white empty boxes" instead of valid icons are related to a non supported font on a given device. In the past some devices didn't support Roboto Medium (but this also affected regular text, not only icons). Using an alternative font used to fix this. For instance https://github.com/devoxx/MyDevoxxGluon/issues/212 was some of this cases. In any case, do you have any logs that could point to a font issue? Or do you have a small test to reproduce the issue? Does it happen in other Android devices?

javateer commented 1 year ago

@johanvos @jperedadnr , thank you, both of you, for being so fast on the draw with your replies. I have a fix suggestion for @nlisker.

The current POM file of the demo project we're referring to has the following settings in it:

<gluonfx.plugin.version>1.0.12</gluonfx.plugin.version>
<attach.version>4.0.15</attach.version>

I see in another project of mine I don't have the Xed boxes issues and that other project has these values for the same settings:

<gluonfx.plugin.version>1.0.16</gluonfx.plugin.version>
<attach.version>4.0.16</attach.version>

Now, no more boxes. Note - this screencapture below is from my Android, not Desktop. However, it is from a Pixel 6, not a Samsung. Still, though this issue #1184 is primarily about Samsung list selection(s) not working unless using my demo project's coding solution, I don't believe the Xed box icons issue is Samsung specific. You should see the icons like I'm showing you now from my phone's screen capture.

@jperedadnr, to not bury the primary reason for why #1184 exists by this side chatter about Xed boxes for icons, please take notice that earlier above @nlisker verified my demo code does indeed demonstrate how a Gluon Developer can augment the Gluon software library so that list selections work on all devices, including Samsung. I have seen people report this Gluon-Samsung list selection problem on StackOverflow since many years ago. I hope you're excited to know someone (modest me) has finally solved it.

@nlisker's last remark in his earlier comment above is the question, how do you suggest we proceed? I also have been waiting to learn whether you, @johanvos, and the others that maintain the GluonFX codebase will refactor the code accordingly with the insight discovered by my demo project's working code or if everyone should just choose whether to apply the same adaptive code on top of the Gluon library as I have done in my demo project.

Screenshot_20230314-104443

nlisker commented 1 year ago

The current POM file of the demo project we're referring to has the following settings in it:

<gluonfx.plugin.version>1.0.12</gluonfx.plugin.version>
<attach.version>4.0.15</attach.version>

I see in another project of mine I don't have the Xed boxes issues and that other project has these values for the same settings:

<gluonfx.plugin.version>1.0.16</gluonfx.plugin.version>
<attach.version>4.0.16</attach.version>

Now, no more boxes.

My problem in that regard is https://github.com/gluonhq/attach/issues/341. I can't upgrade to a new Attach version because it breaks Android 8.

@jperedadnr, to not bury the primary reason for why #1184 exists by this side chatter about Xed boxes for icons, please take notice that earlier above @nlisker verified my demo code does indeed demonstrate how a Gluon Developer can augment the Gluon software library so that list selections work on all devices, including Samsung. I have seen people report this Gluon-Samsung list selection problem on StackOverflow since many years ago. I hope you're excited to know someone (modest me) has finally solved it.

@nlisker's last remark in his earlier comment above is the question, how do you suggest we proceed? I also have been waiting to learn whether you, @johanvos, and the others that maintain the GluonFX codebase will refactor the code accordingly with the insight discovered by my demo project's working code or if everyone should just choose whether to apply the same adaptive code on top of the Gluon library as I have done in my demo project.

If Johan and/or Jose can integrate @javateer's fix we could close this issue.

johanvos commented 1 year ago

@nlisker We're looking into it.

jperedadnr commented 1 year ago

I've built and deployed @javateer apk (from https://github.com/javateer/gluonfx-list-selection-adapter).

Checking the changes, I see these are applied to regular ListView and also to CharmListView (Gluon Mobile). We could easily modify the latter, but doing the changes on OpenJFX for ListView (even if it is only for Android) is not that straightforward...

jperedadnr commented 1 year ago

@nlisker, @javateer After some testing, I've simplified the approach from https://github.com/javateer/gluonfx-list-selection-adapter, to this simple class, that works fine on a Samsung device, without any changes to OpenJFX or Gluon Mobile:

import com.gluonhq.charm.glisten.application.AppManager;
import com.gluonhq.charm.glisten.control.AppBar;
import com.gluonhq.charm.glisten.mvc.View;
import com.gluonhq.charm.glisten.visual.MaterialDesignIcon;

import java.util.Optional;

import javafx.collections.FXCollections;
import javafx.event.Event;
import javafx.fxml.FXML;
import javafx.scene.Node;
import javafx.scene.control.ListView;
import javafx.scene.control.SelectionMode;
import javafx.scene.input.MouseEvent;
import javafx.scene.input.ScrollEvent;

public class MainPresenter {

    @FXML
    private View main;

    @FXML
    private ListView<MaterialDesignIcon> listView;

    private boolean isScrollingFinished = false;

    public void initialize() {
        listView.setCellFactory(p -> new IconListCell());
        listView.setItems(FXCollections.observableArrayList(MaterialDesignIcon.values()));
        listView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
        listView.addEventFilter(MouseEvent.MOUSE_PRESSED, e -> {
            // On most of the Android devices, MousePressed event generates the selection 
            // However, on Samsung devices, this fails most of the times, and only some events make it.
            // To fix this, we just consume the event here, and use MouseClick events instead, which are 
            // always working 
            e.consume();
        });
        listView.addEventFilter(MouseEvent.MOUSE_CLICKED, e -> {
            if (isScrollingFinished) {
                isScrollingFinished = false; // reset for User's next possible scroll event.
            } else {
                process(e);
            }
            e.consume();
        });
        listView.setOnScrollFinished((scrollEvent) -> isScrollingFinished = true);
    }

    void process(MouseEvent e) {
        findCell(e).ifPresent(cell -> {
            MaterialDesignIcon icon = cell.getIcon();
            int index = listView.getItems().indexOf(icon);
            if (index > -1) {
                // this is a custom multiple selection implementation. Selecting a new item doesn't deselect previous ones.
                // Deselection is done by selecting twice an item
                if (listView.getSelectionModel().getSelectedItems().contains(icon)) {
                    listView.getSelectionModel().clearSelection(index);
                } else {
                   listView.getSelectionModel().select(icon);
                }
            }
        });
    }

    private Optional<IconListCell> findCell(MouseEvent event) {
        Node node = (Node) event.getTarget();
        IconListCell listCell = null;
        while (listCell == null) {
            try {
                listCell = (IconListCell) node;
            }
            catch (ClassCastException cce) {
                node = node.getParent();
                if (node == null) {
                    break;
                }
            }
        }
        return Optional.ofNullable(listCell);
    }

}

Does that make sense? @nlisker Would that be of any help? Can you test if that works for you?

nlisker commented 1 year ago

Does the original FXML file need to be modified to be suitable for this change of the class?

jperedadnr commented 1 year ago

I just posted a possible implementation. The FXML file for this case is just a View with a ListView control. But you can adjust it to your needs.

nlisker commented 1 year ago

I tested it and it works well. Selection is easy and the scrolling works.

One thing I noticed is that the acceleration between scrolls is not preserved (doing a scroll while the list is scrolling from a previous event resets the acceleration instead of adding to it). I think it's unrelated and that this behavior exited before also.