MEGA65 / mega65-core

MEGA65 FPGA core
Other
240 stars 85 forks source link

RC 0.95: Configure time set doesn't always stick while editing #613

Closed dansanderson closed 1 year ago

dansanderson commented 1 year ago

Test Environment (required)

Describe the bug Release candidate 0.95, Configuration v01.00: new time setter may or may not retain the value I type. This appears to be an issue with how the clock is being updated in the field while I'm typing in it. When I leave the field late, sometimes it re-reads it from the RTC and overwrites my value.

I can't tell under what circumstances it retains the value and when it doesn't. I was able to set my hour field with some struggle, but I have not successfully set my minute field yet.

Important: I am running a Grove external RTC. My batch #1 MEGA65 has a non-functional internal RTC (no ticks).

To Reproduce Steps to reproduce the behavior:

  1. Install core dadad1c in slot 1.
  2. Power off, hold Alt, power on. Select Configuration (1).
  3. Use cursor keys to navigate to Chipset menu, with Time field selected. Notice it is updating the current value once per second.
  4. Enter a different hour value from what is set. Press Tab to navigate to the minute field at different times.

Expected behavior If I have typed any value in a time field, it should retain that value and set it to the RTC when I leave the field, regardless of when I leave the field.

Additional information I would further suggest that typing two digits in any time field should automatically advance the cursor to the next field. Right now I have to type (for example) 2 3 TAB 2 8 TAB 0 0. I personally wouldn't mind freezing the entire Time value if any Time field is active.

Pressing TAB with the seconds field active does nothing. Maybe this should advance to the first Date field?

dansanderson commented 1 year ago

I'm having other issues with the Time and Date setting in Configuration retaining their values. I just set my Date to 31-OCT-2022, cursored to another tab, then cursored back and it went back to its previous date value.

Again, this is with a Grove external RTC and the dadad1c candidate core in slot 1.

M3wP commented 1 year ago

I believe that this is a problem with not having enough documentation.

The date and time fields aren't saved in the same way as the other details. This is because they aren't restored by Hyppo during the boot process, they are simply free-running.

In order to save the time or date you enter, you need to press "Return" while editing the field. This can be done at any time but when it is done, validation is also applied so invalid times or dates will be altered.

Tab is required to shift between the sub-fields of the time field because the sub-field (hours, minutes, seconds) being edited is locked from updates until moving to another field or sub-field. This could be changed but I do not recommend it. Some "fussing" or waiting may be required to get the exact number of seconds required and this is why the sub-field does not move automatically.

Locking the entire time field when editing would be possible but would not look nearly as pretty and would likely be confusing since it is the first field on the page. This is not recommended. The date field is already locked in this way.

Tab does not move to the next field. Use the cursor up/down keys to do this. This could be changed but it would likely need to be made a general key and I again, do not recommend it.

dansanderson commented 1 year ago

There are more fundamental UX issues here that can't be patched over by documentation.

The requirement to press Return is extremely unintuitive, not least because pressing Return appears to do nothing at all: it doesn't advance the cursor to another field, and there's no other user feedback if the user has entered a valid date. If I change a date value then use the cursor keys to navigate to other fields on the page, the date field appears to retain the value I entered. If I leave the Chipset page, the value I entered is lost, and I may never notice. There is nothing to indicate that the value is unsaved and it's my responsibility to do something special to save it, and there is nothing in the entire UI that needs the Return key for this purpose.

I agree that part of the problem is we're mixing UI metaphors. The overall Config interface behaves as a draft document that doesn't take effect until explicitly saved in the Done tab. The date and time fields do not currently behave this way and are updating the RTC during the editing process. It also needs value validation, and there's no other part of this UI that currently validates values. If space weren't at a premium, I'd suggest that the date and time be a modal editing interface (e.g. a pop-up window) that reinforces that settings are validated and set on exit; exit can be blocked with a message if the value is invalid, with an option to revert and cancel.

We should at least validate and save/restore the date the moment the field is exited via cursor up/down or the page is exited via cursor left/right, i.e. all loss of focus should behave like Return. It's less than ideal that an invalid date value is invisibly reverted when I leave via cursor left/right, but fixing that would require bigger changes to the overall Config UI. As is, I can enter a valid date value and have it invisibly reverted, which is unacceptable.

M3wP commented 1 year ago

There will be no modal functions, no pop-ups.

Why is updating a value by pressing Return "unintuitive"? What sort of feedback is actually required? These fields are quite specifically different to the others.

Why do you need auto-commit? What if you have entered the incorrect value and don't want it anymore? Another "unintuitive" key press (labelled so because you don't know it yet) to reset?

It will only "revert" your entry if you don't "commit" it. All I can see is user error in light of not having the correct documentation. There is no "fundamental" problem only a lack of familiarity of use.

If you enter a valid value and commit it, your problem is solved.

dansanderson commented 1 year ago

+1 to not doing a modal pop-up here, there's not enough time or space. :)

There are only three text fields in the entire Config UI, so there's not a lot of opportunity for a user to discover how it works. The Default Disk Image field retains its value without pressing Return. It's only the date and time fields that use Return in this way.

I can only say that the current behavior is against my expectations and I'm not sure how I could have figured it out. If I'm in the Hour field and type a value then press Tab to navigate to the Minute field, the Hour field reverts to its previous value. As far as I can tell, I can't edit the time. Now that you've told me how it is implemented, I see that I can enter the Hour field value and press Return, then press Tab, then enter a Minute value and press Return, etc. Pressing Return doesn't appear to do anything, even though it is doing something. I would at least expect to be able to enter the entire time value before needing to press Return to set it.

The date field appears to behave differently from the time field in this way. I can tab through its fields and enter values without pressing Return, and they don't revert until I switch pages.

If we really need the user to press Return within each sub-field to commit its value, we should print a "press return to set" message under the selected field if the field contains changes (and hide it when the field doesn't contain changes). The time and date fields should behave consistently with each other. Does that sound right?

M3wP commented 1 year ago

Yes, you were meant to be able to enter the whole time value before commit. At some time during the development, this was lost. This should be addressed to improve ease-of-use.

There is no commit required on the Disk Image field because it is managed by the standard save and Hyppo load. The MAC address field also behaves differently to the others but this seems to have gone unnoticed.

There is absolutely no room for any messages. I don't think I can even change the colour of the field at this point in time.

If you change the date and time, some kind of commit is going to be required. I think auto-commit would be a mistake.

lydon42 commented 1 year ago

The ticking while editing has been disabled. Pressing RETURN to write value entered into RTC is needed.