Conversation
…ed) bugs Fixes issue nightscout#1115. Replaces PR nightscout#1365 which I accidentally screwed up when attempting to process the feedback. Issue summary: The treatments menu would, when tapping the bolus screen, not scroll up properly on some devices. It would stay behind the keyboard. This PR: 1.) fixes the problem and scrolls the focused field up properly. The first attempt (see aforementioned PR) relied on a hard-coded delay, about which feedback was given that it shouldn't be done that way. Therefore, this version is event driven/notification driven and tracks states. 2.) Both in the previous version as this one: after the initial changes, the swift compiler would crash on a too large body (causing type-checker limit violation). To accommodate for this, the Reduced Bolus/Super Bolus toggles + Recommendation button + Bolus field + External Insulin toggle were extracted into their own "@ViewBuilder private var bolusSection: some View", called from a single line in place of the original inline block. No logic should've been altered. 3.) Fixed a naming collision that occurred. 4.) Fixes a staleness bug in the queued-scroll state. "pendingScrollTarget" could be left set if focus cleared before the keyboard ever showed, and - since "keyboardWillShowNotification" fires app-wide, not scoped to this view - a later, unrelated keyboard appearance could act on that stale value and trigger an unexpected scroll. This was fixed two ways: "pendingScrollTarget" is now cleared whenever focus becomes nil, and "handleKeyboardWillShow" re-validates that the queued target still matches the currently focused field before acting on it. Also cleared defensively on "keyboardWillHide" as a second safety net. 5.) Somehow, perhaps due to some re-arranging, it also fixes the problem that would cause the entire app to hang for +/- 10 seconds when a field was tapped for the first time whilst the app was running with the debugger attached. That wasn't the goal, but won't complain. All of the above lives entirely within this file, confined to the scroll-related code. None of it should touch the bolus dosing logic or the Reduced Bolus/Super Bolus toggles' behavior. The code has been generated and verified with the help of Claude and ChatGPT. Initial testing performed in production on iOS 26 on dev d1a66f4 by myself over the course of the last couple of weeks. Everything appears to be working as intended, no errors triggered and all fields behave as intended.
|
Let’s please largely cut back on the prosa-like comments / docstrings. |
Clean up the comments
|
@trioneer-dev Trimmed down the comments, rest is untouched :) Tested on device to verify it still works. |
|
Could you resolve the branch conflicts @LiroyvH ? Then I can test and approve this. |
|
@t1dude Never dealt with that before. Looking at it now, GitHub proposes to either accept current state, accept incoming change or accept both changes. But looking at it, I don't think any of those will actually work... Because it'll either a.) remove the what appear to be accessibility changes that have interim been merged in to this file in dev, or b.) re-introduce the compiler issues that were resolved or c.) it'll render the bolus section twice (and probably also fail to compile lol) So I guess there is no way to resolve this using GitHub's proposals. I'll see if I can submit another manual change to the file, but I'm not sure what happens if I manually add these changes whilst the target branch (dev) doesn't require some of those changes as it actually introduced them. If it screws it up I'll just have to open a new PR. |
|
@t1dude So tried in a different way, but the conflict resolution editor doesn't show a diff on commit - I had expected it to show that before actually committing... So nope, that didn't go quite right and now the file isn't quite right/partially resolved. I'll update it again tomorrow. It should not be merged now as that undo's some of the changes. |
|
It's a common issue when several people are working on the same files. I usually ask Claude to help with merge conflicts like this and then I review the changes. Manually it just takes too much time. |
Latest changes to match it with current dev (accessibility) and add the new auto-scrolling, reduced comments and smaller block to fix compiler issue
|
@t1dude Alright this should be fixed now, everything that was changed interim (accessibility labels and some changes for liquid glass by the looks of it) should be retained properly now - conflicts resolved. Ran final test on this file on my production build: works as intended, including the scrolling. To be complete, this PR then should now be in-line with the current state in dev and only adds all the changes + fixes mentioned in the first message in this PR. |
Fixes issue #1115. Replaces PR #1365 which I accidentally screwed up when attempting to process the feedback.
Issue summary:
The treatments menu would, when tapping the bolus screen, not scroll up properly on some devices. It would stay behind the keyboard.
This PR:
1.) fixes the problem and scrolls the focused field up properly. The first attempt (see aforementioned PR) relied on a hard-coded delay, about which feedback was given that it shouldn't be done that way. Therefore, this version is event driven/notification driven and tracks states.
2.) Both in the previous version as this one: after the initial changes, the swift compiler would crash on a too large body (causing type-checker limit violation). To accommodate for this, the Reduced Bolus/Super Bolus toggles + Recommendation button + Bolus field + External Insulin toggle were extracted into their own "ViewBuilder private var bolusSection: some View", called from a single line in place of the original inline block. No logic should've been altered.
3.) Fixed a naming collision that occurred.
4.) Fixes a staleness bug in the queued-scroll state. "pendingScrollTarget" could be left set if focus cleared before the keyboard ever showed, and - since "keyboardWillShowNotification" fires app-wide, not scoped to this view - a later, unrelated keyboard appearance could act on that stale value and trigger an unexpected scroll.
This was fixed two ways: "pendingScrollTarget" is now cleared whenever focus becomes nil, and "handleKeyboardWillShow" re-validates that the queued target still matches the currently focused field before acting on it. Also cleared defensively on "keyboardWillHide" as a second safety net.
5.) Somehow, perhaps due to some re-arranging, it also fixes the problem that would cause the entire app to hang for +/- 10 seconds when a field was tapped for the first time whilst the app was running with the debugger attached. That wasn't the goal, but won't complain. (Not that it was a major issue... merely annoying when running XCode debugger.)
All of the above lives entirely within this file, confined to the scroll-related code. None of it should touch the bolus dosing logic or the Reduced Bolus/Super Bolus toggles' behavior or anything like that.
Full-disclosure:
The code has been generated and verified with the help of Claude and ChatGPT. Initial testing performed in production on iOS 26 on dev d1a66f4 (the previous version was tested on other dev versions obviously) by myself over the course of the last couple of weeks. Everything appears to be working as intended, no errors triggered and all fields behave as intended.