[GSoC'24] Introducing AnalyticsWidgetProvider as ancestor of all widget classes.

[xenonnn4w]

Purpose / Description

Adding Up Analytical widget provider class for Widgets in AnkiDroid .

[GSoC'24] Implementation of Deck Picker Widget . #16450

Preview of affected UI areas after code changes [Functioning as Expected ]

AnalyticalWidgetProviderTest.mp4

Checklist

Please, go through these checks before submitting the PR.

• You have a descriptive commit message with a short title (first line, max 50 chars).
• You have commented your code, particularly in hard-to-understand areas
• You have performed a self-review of your own code
• UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
• UI Changes: You have tested your change using the Google Accessibility Scanner

[david-allison]

You should use this with our current classes

[xenonnn4w]

You should use this with our current classes

Sorry, didn't understand what you meant by that

[david-allison]

Two widget classes appear to be using this functionality and have duplicated code.
You're planning on adding a third which also uses this functionality, so the concern was extracted to a class

You should add the class which you've created into the inheritance hierarchy for the two other classes to reduce complexity

[david-allison]

LGTM, cheers

[Arthur-Milchior]

I don't understand you "how has this been tested" section. Can you please explain how the link refers to tests ?

[david-allison]

Pinged Arthur for re-review

[Arthur-Milchior]

There are some changes I don't understand. I am not 100% certain they are wrong, I may misunderstood things. But I'd really like your commit message to explain changes that don't seem to be directly related to refactoring common code.

My suspicion is that you did git pull origin main and didn't resolve properly the merge conflicts

[Arthur-Milchior]

Honestly, I'm unhappy with the recent changes.
Maybe they are correct change, maybe they are wrong change, I honestly don't know, because you don't explain what you're doing.

I spent more than an hour on it, trying to explain in details what's wrong. This should not be the case on a commit that is a month an a half old. Especially given that I already asked you the previous time to add comment and documentation, to explain to us what you're doing.

David did approve a previous version of the PR. Personally, I had some requests, but it seems globally okay. Now you made many you changes, you MUST explain why you did them. When I came back to review your PR, I was expecting to see essentially the same PR than last time, with the changes I requested and maybe change requested by David. If David approved your PR, you are not supposed to change it, or if you must, you should ask for a new review, or explain that it's really a trivial change (for example you rebased onto main, or you found a typo in a comment you wanted to fix).
You made really non trivial change. If they are really things you wanted to do, that you believed should be merged with the current PR, then you should have written it down explicitly, you should have explained what was changed so that we don't need to re-read everything, and check from scratch whether every part of the PR is okay in this new environment.

And, even if it were a fresh PR and not an update on a PR that was already reviewed, you need to save us time. I don't know Widgets very well, I don't know the details of AppWidgetManager. I should not have to know them. Android is huge, and maintainers do not have to know the entire system. The contributor that does a change, especially in the case of GSoC where it's your responsability to do this change, must assume that we know Kotlin correctly, that we have an idea of how Android generally works. But it's your role to explain what you do so that we don't have to read the whole documentation to understand the reason behind your proposed changes. And similarly, every such non trivial changes must be explained in the commit message, to help future reader of the code understand why the code is written the way it is, and maybe revert the change, or debug it, if it's not what you expected.

As far as I'm concerned, I will not accept to review another PR of yours unless every non trivial change you did are documented in the commit message, or comes with a comment or documentation near it.
If you have a doubt whether a change is trivial or not, consider it to be non-trivial. Once you document too much, we'll discuss or to ensure you comment just enough. But first, you must take the habit to comment a lot.

Finally, regarding

for some reason widget_eta and widget_due are not rendering after these changes , was investigating that onl;y

Here are my requests:

• either you repaired it, and that's great. Please let us know
• or it's still broken. In this case, please provide screenshot, so we understand exactly what you mean here. What is broken. Maybe we can help you. And, more importantly, either you believe the PR can still be merged, in which case explain why it's okay to merge this new class, even if it have issues. Or it can't be merged, and change the PR so that it moves to Draft, so that it's clear that there are still things wrong here. And please communicate with us, don't hesitate to ask for help. We didn't even knew there was an issue before you mentioned it as a reply to David. And if there is an issue with code we approved, it means we missed some errors, that's something that is interesting for us to know

[david-allison]

Looks good, cheers

[Arthur-Milchior]

Regarding the commit message, the way it usually work in github and most git tool is that the first line is considered to be the title, and the remaining is the body of the message.

So I'd appreciate if you could have a first line with something that looks like a title. The title will be the only thing that someone will see when they look at the history of AnkiDroidWidgetSmall.kt 's git blame for example.

It can be something as simple as:

Introducing AnalyticsWidgetProvider as ancestor of all widget classes

And then you can add a small message that explain why it's done, so that anyone looking at the commit understand more of the context.

This will ensure any widgets Timber and send analytics on enabling, disabling and updating.

[github-actions]

Hi there @xenonnn4w! This is the OpenCollective Notice for PRs merged from 2024-07-01 through 2024-07-31

If you are interested in compensation for this work, the process with details is here:

https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

Important

PLEASE NOTE: The process was updated in August 2024. Re-read the Payment Process page if you have not already.

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!