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

Preselect correct time in session form #31

Closed
rolandgeider opened this issue Jun 5, 2021 · 25 comments · Fixed by #93
Closed

Preselect correct time in session form #31

rolandgeider opened this issue Jun 5, 2021 · 25 comments · Fixed by #93
Assignees
Labels
good first issue Good for newcomers

Comments

@rolandgeider
Copy link
Member

The session form (last page in the gym mode) always pre fills the current time when editing the start or the end time. It should of course use the values present in the field

@rolandgeider rolandgeider added the good first issue Good for newcomers label Jun 5, 2021
@TomerPacific
Copy link
Contributor

@rolandgeider , can I help out with this issue?

@rolandgeider
Copy link
Member Author

all yours

@TomerPacific
Copy link
Contributor

@rolandgeider, after upgrading my flutter version and installing all the dependencies, I am trying to run the application and getting the following:

../../Flutter%20SDK/flutter/.pub-cache/hosted/pub.dartlang.org/charts_flutter-0.11.0/lib/src/behaviors/legend/legend_entry_layout.dart:134:45: Error: The getter 'body1' isn't defined for the class 'TextTheme'.
 - 'TextTheme' is from 'package:flutter/src/material/text_theme.dart' ('../../Flutter%20SDK/flutter/packages/flutter/lib/src/material/text_theme.dart').
Try correcting the name to the name of an existing getter, or defining a getter or field named 'body1'.
      color ??= Theme.of(context).textTheme.body1!.color;
                                            ^^^^^

Any idea why this is happening? It seems like something custom to your application as there is no such thing as body1. Maybe you meant bodyText1?

@rolandgeider
Copy link
Member Author

for a change that's not on us, but on one of the dependencies 😅

You can use the 2.2.x version till they release an updated version

@rolandgeider
Copy link
Member Author

I mean flutter 2.2.x

@TomerPacific
Copy link
Contributor

TomerPacific commented Oct 4, 2021

Yeah, I got you. Would you like me to open an issue for this?

@TomerPacific
Copy link
Contributor

Trying to run flutter downgrade to any version that is 2.2.x and getting:

C:\Users\Tomer\Documents\Projects\flutter>flutter downgrade v2.2.1
Downgrade flutter to version 2.0.2

It defaults to downgrade to version 2.0.2.

@rolandgeider
Copy link
Member Author

that's strange, are you on the stable channel?

If that doesn't work, there is a workaround to make the package work with 2.5, just don't commit that: google/charts#678 (comment)

@TomerPacific
Copy link
Contributor

@rolandgeider, yes I am on the stable channel. Ran flutter doctor and this is the output:

[√] Flutter (Channel stable, 2.5.2, on Microsoft Windows [Version 10.0.19043.1237], locale he-IL)
[!] Android toolchain - develop for Android devices (Android SDK version 30.0.3)
    X cmdline-tools component is missing
      Run `path/to/sdkmanager --install "cmdline-tools;latest"`
      See https://developer.android.com/studio/command-line for more details.
    X Android license status unknown.
      Run `flutter doctor --android-licenses` to accept the SDK licenses.
      See https://flutter.dev/docs/get-started/install/windows#android-setup for more details.
[√] Chrome - develop for the web
[√] Android Studio (version 4.0)
[√] IntelliJ IDEA Community Edition (version 2019.3)
[√] VS Code (version 1.60.2)
[√] Connected device (3 available)

I'll use the workaround you suggested (and won't commit it).

@rolandgeider
Copy link
Member Author

BTW, you will need the backend server for this. If you don't want to setup everything with docker I can give you access to the test server, just write me an email

@M123-dev
Copy link
Contributor

M123-dev commented Oct 4, 2021

Trying to run flutter downgrade to any version that is 2.2.x and getting:

C:\Users\Tomer\Documents\Projects\flutter>flutter downgrade v2.2.1
Downgrade flutter to version 2.0.2

It defaults to downgrade to version 2.0.2.

Same thing happend for me, I also ended up using the workaround, but it seems like it worked when downgrading when in master channel.

@TomerPacific
Copy link
Contributor

@rolandgeider, since I am using a Windows machine, I had previous problems running docker on it. I'll write you an email so I could use the test server. Thanks.

@rolandgeider
Copy link
Member Author

Just sent you the login. And yeah, Docker in windows has gotten much better, but it still doesn't feel completely native.

(use a day with few exercises, otherwise you'll have to click yourself to death to get to the end of the gym mode)

@TomerPacific
Copy link
Contributor

TomerPacific commented Oct 4, 2021

@rolandgeider, now that I have everything up an running I can better understand what is desired in this issue.
When talking about the session form, I am guessing you are referring to this page:

qemu-system-x86_64_JTLOKv57eC

As stated, it uses the current time of the device. When you say it should instead use the values preset in the field, what do you mean? What would you like to be preset there?

@rolandgeider
Copy link
Member Author

Yeah, that's the one. If you change the time and click to edit, the current time is selected again

@TomerPacific
Copy link
Contributor

TomerPacific commented Oct 4, 2021

@rolandgeider. got it. I thought you want other values to show up when the screen is first shown.
Testing this out on an emulator device, I am getting mixed results.
It does seem to be working.

oATK2k7rYA.mp4

Is there a specific scenario?

@rolandgeider
Copy link
Member Author

I meant that if you set the time and click to edit it again the pre-selected time is always "now", regardless of what is on the field

@TomerPacific
Copy link
Contributor

@rolandgeider, now I understand completely. The issue is probably because:

// Open time picker
                            final pickedTime = await showTimePicker(
                              context: context,
                              initialTime: widget._start,  /// <-----------
                            );

and

 final pickedTime = await showTimePicker(
                            context: context,
                            initialTime: TimeOfDay.now(),  /// <------------
                          );

I thought about adding a ternary that does the following:

initialTime: _session.timeStart != null ? _session.timeStart : widget._start

Because these two fields are initialized late, I can't just check their value (it throws an exception).

What would you prefer that we do?

  • We can give these fields a null starting value and check for that
  • Wrap the check for these fields with try/catch
  • Add boolean fields to check the status of these fields (initialized or not)

Each option has it's pros/cons. Let me know what you think would work best.

@rolandgeider
Copy link
Member Author

I'm thinking, when is _session created anyway? Can't we just make sure it always have a start and end time?

@TomerPacific
Copy link
Contributor

TomerPacific commented Oct 6, 2021

@rolandgeider,
Session is initialized here (inside gym_mode.dart):

class _SessionPageState extends State<SessionPage> {
  final _form = GlobalKey<FormState>();
  final impressionController = TextEditingController();
  final notesController = TextEditingController();
  final timeStartController = TextEditingController();
  final timeEndController = TextEditingController();

  final _session = WorkoutSession();

The constructor leads to the session.dart class, which is a representation of a session with all the fields (which are declared late).
The session data itself is being fetched from workout_plans.dart:

  Future<dynamic> fetchSessionData() async {
    final data = await fetch(
      makeUrl(_sessionUrlPath),
    );
    return data;
  }

But (using the mock sever data), when fetching a session that does not exist on the server, I am guessing that all the fields are left uninitialized as they are marked late. As I suggested earlier, we can give certain (or all) fields a default value.

@rolandgeider
Copy link
Member Author

so if we initialise the fields with the current time we won't need any more checks for the popup later on (don't have the code in front of me right now). That sounds like a much cleaner solution than checking for null or similar

@TomerPacific
Copy link
Contributor

TomerPacific commented Oct 6, 2021

@rolandgeider, ok, just figured out the root cause of all of this. The field timeStart/end is only set on the onSaved callback, which gets called when the user presses the Save button.
So, to fix it, all you need to do is:

                           // Open time picker
                            final pickedTime = await showTimePicker(
                              context: context,
                              initialTime: _session.timeStart,  /// <---------
                            );

                            if (pickedTime != null) {
                              timeStartController.text = timeToString(pickedTime)!;
                              _session.timeStart = pickedTime;  /// <-----------------
                            }

The same goes for timeEnd

WDYT?

@rolandgeider
Copy link
Member Author

even simpler :) Go ahead with this

@TomerPacific
Copy link
Contributor

@rolandgeider, I have opened a PR (after testing the solution). Would you mind adding the hacktoberfest-accepted label to this issue?
Also, in the PR template it says to add myself to the authors.rst file? I found the authors.md file and didn't know if I should add myself to it.

@rolandgeider
Copy link
Member Author

oh yes, I meant authors.md, I'll fix the text. If the repo has the hacktoberfest tag the individual PRs don't need it as far as I know

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants