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

Event log access from MegaStatus menu #2159

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Lupum1001
Copy link
Contributor

@Lupum1001 Lupum1001 commented May 30, 2022

This PR is based on the following discussion/request: #2128

it enables the dropdown menu for MegaStatus, and adds a link to the event log, just like in the Home activity.


***UPDATE: I've removed the following refactoring part and put it in its own PR: #2293

I also did a bit of refactoring in Home.java->onOptionsItemSelected(), based on the following linting advice:
image
I got rid of the initial switch-case and converted everything into one big if-else-if statement.

@Lupum1001 Lupum1001 marked this pull request as ready for review May 30, 2022 01:56
Copy link
Collaborator

@tolot27 tolot27 left a comment

Choose a reason for hiding this comment

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

Currently, we cannot upgrade the AGP because of the wrong casing of package names (#1522) and it is unlikely that this will change in the near future because we had to modify a lot of classes (#1539).
Hence, I would separate this into another PR which could contain all fixes for this lint waring.

@Lupum1001
Copy link
Contributor Author

Hence, I would separate this into another PR which could contain all fixes for this lint waring.

I figured I might as well change it while I was already there, but there are some efficiency improvements in there as well.

  • item.getItemId() only really needs to be called once
  • if an 'if' statement evaluates as true, the use of else-ifs prevents the subsequent checks from having to run anyway.

In theory, I actually prefer a switch-case for the whole function (replacing the 'if' statements), because I feel it's cleaner syntax when you're just checking the id against a bunch of different options. I figured it was better to go with the future-proof format though.

@jamorham
Copy link
Collaborator

jamorham commented May 30, 2022

If they're going to be fussy about resource id's (and I don't know why the integer can't be used in a switch statement) then wouldn't it be better just to use the onClick xml parmeter instead as this is closer to the databinding ui method which is used in later parts of the code anyway and is less susceptible to merge conflicts than an if/else chain? (I also like this because you can call it with null and use the utility of that menu item to trigger programatically)

Enabling the overflow menu in MegaStatus, and adding a link to view the event log. This was based on the following discussion/request: NightscoutFoundation#2128
@Lupum1001 Lupum1001 force-pushed the feature/log_access_from_megastatus_menu branch from 1328808 to f758022 Compare August 25, 2022 06:31
Lupum1001 added a commit to Lupum1001/xDrip that referenced this pull request Aug 25, 2022
Lupum1001 added a commit to Lupum1001/xDrip that referenced this pull request Aug 25, 2022
@Lupum1001 Lupum1001 force-pushed the feature/log_access_from_megastatus_menu branch from c0c8f38 to f758022 Compare August 25, 2022 08:01
@Lupum1001
Copy link
Contributor Author

Ok, I've moved the refactoring out of this PR; it just includes the new menu item now.

PR with the refactoring coming momentarily.

@Lupum1001 Lupum1001 requested a review from tolot27 September 11, 2022 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants