Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the dateinput and timeinput widgets for toga/web #2176

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

dgmouris
Copy link
Contributor

@dgmouris dgmouris commented Oct 27, 2023

Describe your changes in detail

Adds a date input widget for the toga on the web.

I'm looking for a bit of an understanding of how to contribute to the web part of this project as well.

What problem does this change solve?

Adds the DateInput widget for toga web.

Issues related

Testing web applications

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@dgmouris
Copy link
Contributor Author

I know this is a small PR, but it's more so that I can understand a bit more on how you folks want to get this tested.

The testbed for the web is being worked on, but I'd love to hear how you folks have tested in the past.

Thanks so much for the cool project folks!

@freakboy3742
Copy link
Member

At the moment, the testing regimen for web widgets is to run the example app that exercises the widget, and poke around a bit. That's obviously a weak test, but a partially working something is better than nothing :-)

If you want to be extra rigorous, you can look at the what the testbed is verifying, and try to reproduce the same set of behaviours manually.

I'll take a closer look when I'm back at work next week - but from a quick inspection of the code, this looks like it's on the right track.

Getting the testbed working on web would be the ideal goal; however, that's a little complicated to get working because of the limitations of WASM as a platform. In particular, there's no threading, and the current test harness uses threading to coordinate test execution. I have some ideas on how the issues could be resolved, but it's going to require some fairly big changes to how the rest of the testbed suite runs. If you're potentially interested in digging into the problem, I can go into more detail.

@dgmouris
Copy link
Contributor Author

Hey thanks for the response.

So what I've inferred here is that I should go to examples/date_and_time and the way I should run these example is the following:

pip install briefcase
briefcase run web -u

I just add the -u to get the updated version...

As for the text execution for the WASM I hear you on some the complexity. I want to feel the pain of testing in this way before I make suggestions or go digging so I can have a better understanding, but I'm interested in taking a gander at the issue in the future:)

@dgmouris
Copy link
Contributor Author

dgmouris commented Oct 31, 2023

Essentially I've come up with a way to get the debugger consistently so I can test things (rudimentarily).

  1. build and load the project
pip install briefcase
briefcase run web -u
  1. press "c" on the first alert
  2. reload the page and you should be able to debug the application with a breakpoint, by taking a look at the output on the console.

Not exactly the cleanest but I'll continue to chip away at this:)

@freakboy3742
Copy link
Member

pip install briefcase
briefcase run web -u

I just add the -u to get the updated version...

You may also need to use -r to ensure that the requirements (i.e., toga itself) is updated.

As for the text execution for the WASM I hear you on some the complexity. I want to feel the pain of testing in this way before I make suggestions or go digging so I can have a better understanding, but I'm interested in taking a gander at the issue in the future:)

Totally understood - best to gain an understanding of the problem before you try to fix it.

Throwing another idea into the mix - at present, the -r step requires rebuilding wheels for deployment purposes. This slows down the development loop because a single change to a line of code requires a non-trivial rebuild before you can see the impact. With some different initial setup, I think it would be possible to reference the code directly, rather than as a wheel, which should reduce the development cycle to a reload in the browser. This should be a lot less complex than fixing the entire test infrastructure, but would significantly improve the development experience. This might be something that we could incorporate into a web dev mode (say briefcase dev --mode=web) to provide an analog of briefcase dev but for web purposes.

@mhsmith
Copy link
Member

mhsmith commented Nov 1, 2023

It isn't just web where the slow wheel rebuilding is a problem. I do this all the time to speed up testing on Android, with the following patches:

diff --git a/core/src/toga/__init__.py b/core/src/toga/__init__.py
index 705ee52e0..73abccb5f 100644
--- a/core/src/toga/__init__.py
+++ b/core/src/toga/__init__.py
@@ -92,6 +92,7 @@ __all__ = [
 
 
 def _package_version(file, name):
+    return "0.0.1"
     try:
         # Read version from SCM metadata
         # This will only exist in a development environment
diff --git a/core/src/toga/platform.py b/core/src/toga/platform.py
index c867a8cbb..bd3df3a90 100644
--- a/core/src/toga/platform.py
+++ b/core/src/toga/platform.py
@@ -56,7 +56,7 @@ def get_platform_factory():
     """
     toga_backends = entry_points(group="toga.backends")
     if not toga_backends:
-        raise RuntimeError("No Toga backend could be loaded.")
+        pass
 
     backend_value = os.environ.get("TOGA_BACKEND")
     if backend_value:
diff --git a/testbed/pyproject.toml b/testbed/pyproject.toml
index 9107fb175..6e514ed82 100644
--- a/testbed/pyproject.toml
+++ b/testbed/pyproject.toml
@@ -17,9 +17,11 @@ sources = [
 ]
 test_sources = [
     "tests",
+    "../core/src/toga"
 ]
 requires = [
-    "../core",
+    "travertino>=0.3.0",
+    'importlib_metadata>=4.4.0; python_version <= "3.9"',
 ]
 test_requires = [
     "coverage==7.2.0",
@@ -84,9 +86,7 @@ requires = [
 [tool.briefcase.app.testbed.android]
 test_sources = [
     "../android/tests_backend",
-]
-requires = [
-    "../android",
+    "../android/src/toga_android",
 ]
 test_requires = [
     "fonttools==4.42.1",
diff --git a/testbed/tests/testbed.py b/testbed/tests/testbed.py
index 03f983431..570db0207 100644
--- a/testbed/tests/testbed.py
+++ b/testbed/tests/testbed.py
@@ -114,6 +114,9 @@ if __name__ == "__main__":
 
         os.get_terminal_size = get_terminal_size
 
+    # FIXME
+    os.environ["TOGA_BACKEND"] = toga_backend
+
     # Start coverage tracking.
     # This needs to happen in the main thread, before the app has been created
     cov = coverage.Coverage(

Obviously this is a bit awkward. But rather than adding a workaround to Briefcase, it would improve many more situations if we could speed up the wheel building in pip itself, which seems to be unreasonably slow.

And I suspect the reason for the slowness is that when it creates a virtual environment to do the build, it does so with the default settings that install pip and setuptools into the environment, requiring hundreds of files to be unpacked. This should no longer be necessary, now that pip has the --python option which allows a copy of pip installed in one environment to operate on a different environment.

@dgmouris
Copy link
Contributor Author

dgmouris commented Nov 1, 2023

Throwing another idea into the mix - at present, the -r step requires rebuilding wheels for deployment purposes. This slows down the development loop because a single change to a line of code requires a non-trivial rebuild before you can see the impact. With some different initial setup, I think it would be possible to reference the code directly, rather than as a wheel, which should reduce the development cycle to a reload in the browser. This should be a lot less complex than fixing the entire test infrastructure, but would significantly improve the development experience. This might be something that we could incorporate into a web dev mode (say briefcase dev --mode=web) to provide an analog of briefcase dev but for web purposes.

I like the idea of having something equivalent to briefcase dev --mode=web to do some sort of equivalent of "Hot Module Replacement" (HMR) in JavaScript to make the development experience. I know that under the hood, HMR in JavaScript uses websockets which might help implementing the infrastructure for the testbed.

@freakboy3742
Copy link
Member

I like the idea of having something equivalent to briefcase dev --mode=web to do some sort of equivalent of "Hot Module Replacement" (HMR) in JavaScript to make the development experience. I know that under the hood, HMR in JavaScript uses websockets which might help implementing the infrastructure for the testbed.

FWIW - hot reloading would be great for the development experience on every platform, not just web; and just being on web won't automatically make hot reloading easier for Toga, because the thing that needs to be reloaded is a running Python interpreter, not DOM+JS.

It's also a non-trivial problem, as you essentially need to re-create the state of the app pre/post reload. That possibly intersects with a bunch of work we need to do about restoring state after sleep (e.g., mobile apps restoring after being put in the background).

But - if someone wants to look into the problem, I won't stop them :-)

@dgmouris
Copy link
Contributor Author

dgmouris commented Nov 3, 2023

Hey I did some manual testing which is working as expected. The only thing we'll have to fix in the future is the time minimum and maximum as HTML doesn't do the validation so it doesn't respect the changes I'm making.

Here's a gif with some of the testing of the toga/examples/date_and_time application.

temp_beeware

I think this is alright to go for now, let me know if there's any other changes we want to implement. Sorry about this being a tiny test but something is better than nothing.

Thanks again for taking a look at this!

@dgmouris dgmouris changed the title DRAFT Added the dateinput widget for toga/web Added the dateinput and timeinput widgets for toga/web Nov 5, 2023
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've flagged a couple of behavior discrepancies (some of which might be browser dependent); but on the whole, this is looking good.

As an aside - it might also be worth adding a button to the example app that extracts all the dates and times (or, at least, a date and time) so we can easily validate the "get" APIs are working as expected.

@@ -5,7 +5,7 @@ MainWindow,Core Component,:class:`~toga.MainWindow`,The main window of the appli
ActivityIndicator,General Widget,:class:`~toga.ActivityIndicator`,A spinning activity animation,|y|,|y|,,,,|b|,
Button,General Widget,:class:`~toga.Button`,Basic clickable Button,|y|,|y|,|y|,|y|,|y|,|b|,|b|
Canvas,General Widget,:class:`~toga.Canvas`,A drawing area for 2D vector graphics.,|y|,|y|,|y|,|y|,|y|,,
DateInput,General Widget,:class:`~toga.DateInput`,A widget to select a calendar date,,,|y|,,|y|,,
DateInput,General Widget,:class:`~toga.DateInput`,A widget to select a calendar date,,,|y|,,|y|,|b|,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TimeInput will also need to be updated.

changes/2176.feature.rst Outdated Show resolved Hide resolved
def set_value(self, value):
if value is None:
self.native.value = ""
self.native.value = value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't appear to be fully working with initial values. In my testing, on Safari, the current date is being displayed on every date widget in the example app; on Chrome (on macOS), I get dd/mm/yyyy as the default values.

The "with max" and "with min and max" date examples should have an initial date of 2021-04-02; "any" and "with min" should have no initial value (although if the browser defaults to the current date as a browser behavior, that's fine).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is impacting the behavior difference - but I'm using AU date formatting order, so I see "6/11/2023" as todays' date (6 Nov 2023). I'm not sure the extent to which browsers will be rejecting the default value of "%Y-%m-%d" on the basis of localised validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only tested this on chrome, I'll test this on firefox as well to see the differences (my apologies).

The "with max" and "with min and max" date examples should have an initial date of 2021-04-02; "any" and "with min"

but I'll definitely change this piece to have this functionality.

self.native.min = self._format_time(value)

def set_max_time(self, value):
self.native.max = self._format_time(value)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These min/max values don't appear to be applied - I can set times before the min/after the max in the example app.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to know the way you'd like this handled, the way I'm thinking is to essentially set the value for that time to the minimum or maximum (which ever is relevant).

The only thing here is that browsers don't enforce a min/max on the time input and I couldn't find the max/min on the shoelace components.

I'm certainly open to anything here but I think it might be a bit hidden if we set the value here to the max or min time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, Android has a similar limitation. The approach we've taken there is to catch the "on change" event, and when it occurs, explicitly use the API to set the value of the widget to the value that was just set. This might seems like a no-op, but it has the effect of clipping the time to the min/max bounds.

@freakboy3742
Copy link
Member

freakboy3742 commented Nov 6, 2023

One other tip/warning - if you merge this up with main, you'll need to modify the on_change() handlers to remove the first argument. We've been slowing making that first argument redundant as we completed the widget audit; one of the last things we did before landing v0.4.0 was to purge the use of the first argument to event triggers in the codebase.

Depending on when you merge, you may also hit #2194. This will be fixed by #2195, but that hasn't landed yet.

dgmouris and others added 2 commits November 6, 2023 15:34
Co-authored-by: Russell Keith-Magee <russell@keith-magee.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants