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

Fix insulin profile list not showing all insulin profiles #3177

Merged
merged 6 commits into from
Dec 15, 2023

Conversation

TheConen
Copy link
Contributor

@TheConen TheConen commented Nov 7, 2023

As @Navid200 mentioned in #3157, the current insulin profile list has a fixed size and does not show all insulin profiles.

This PR fixes that by changing the fixed vertical size to a dynamic size based on the content.

Before (Notice the missing FIASP which should be at the top:
280917582-8fa0d862-27ee-4fc7-a555-94b31b13aca3

After (depending on the screen size/resolution you need to scroll down to see the buttons):
grafik
grafik

@TheConen
Copy link
Contributor Author

@jamorham Could you have a look at this? I think this is something that should be fixed before the next stable release.

@Navid200
Copy link
Collaborator

There is a (user) workaround.

If the user goes to Android settings and enables the setting that allows the orientation to automatically change between landscape and portrait based on gravity, rotating the phone to landscape mode will allow the user to scroll through the list.

@jamorham
Copy link
Collaborator

Hi, thanks for this. Notes about this PR:

  1. Please integrate the latest changes on master as some of them touch this and there are new support libraries via androidx so please check compatibility.
  2. Please explain the process that this PR uses to fix the problem - I believe you are simply adjusting the layout to use wrap content so that the scrollview contains the full size of the data. But please confirm that is what is happening.
  3. Please remove the changes that have been made to other files. I assume that there are no functional changes to initiallexicon.txt and insulin_profiles.txt and so please remove these files otherwise document the changes and provide a rationale for them that can be reviewed. json files are text files and there is wider ide compatibility using the txt extension afaik.

@TheConen
Copy link
Contributor Author

TheConen commented Dec 11, 2023

1.) Done. Tested it on my OnePlus Nord 2 5G and several devices in Android Studio Emulator. Worked fine on all devices.

2.) The previous layout used two nested ScrollViews, with the inner scroll view being a fixed height of 440dp. This caused problems depending on the screen resolution. I changed this to one ScrollView containing all the content and matching the height to wrap the content, dynamically adjusting to the screen resolution.

3.) The correct file extension for JSON files is .json (see RFC 8259, Section 11 https://datatracker.ietf.org/doc/html/rfc8259). If you use a different file extension, you are losing out on all the support your IDE gives you, like linting, syntax highlighting, auto completion etc. and you are conveying wrong information. JSON is more than just text - it has rules and a syntax that it needs to adhere to. XML and YAML (or all of the Java code, for that matter) are "just" text files, too - but naming your manifest file AndroidManifest.txt instead of AndroidManifest.xml would be wrong (and break your project). There are absolutely no compatibility issues with any IDE or text editor - quite the opposite, you are gaining a lof of support from your IDE by using the correct file extension for your files.
(Fun fact: there is quite a debate about the correct file extension for YAML being .yml or .yaml - according to the official YAML website, it is .yaml https://yaml.org/faq.html)

Also, thank you for updating the configuration so the project works with an up-to-date version of Android Studio now.

@jamorham
Copy link
Collaborator

You make a good argument regarding json file extension. I may be quoting how things were in 2016 with android studio.
Please instead just confirm that, other than the file extension change, the only changes are to whitespace formatting and there are no functional changes to those files and that you have checked that the lexicon file is still being processed correctly?

@TheConen
Copy link
Contributor Author

TheConen commented Dec 13, 2023

There are no changes to the actual content of the file. I just changed the file extension and did an auto-format (Ctrl+Alt+L) with Android Studio.

From what I can tell, everything works fine. initiallexicon and insulin_profiles are both read correctly (I double-checked with the Debugger).

@jamorham jamorham merged commit 60d198e into NightscoutFoundation:master Dec 15, 2023
1 check passed
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