Skip to content

Fix stream notification grouping#12067

Merged
Stypox merged 1 commit intoTeamNewPipe:devfrom
Isira-Seneviratne:Fix-notification-grouping
Feb 28, 2025
Merged

Fix stream notification grouping#12067
Stypox merged 1 commit intoTeamNewPipe:devfrom
Isira-Seneviratne:Fix-notification-grouping

Conversation

@Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Feb 27, 2025

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • It is possible for the stream uploader URL to be different from the channel URL, thus resulting in the notifications not being grouped by the channel. This change fixes that.

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/small PRs with less than 50 changed lines label Feb 27, 2025
@sonarqubecloud
Copy link

@ShareASmile ShareASmile added bug Issue is related to a bug new stream notification Issue is related to notification shown for new streams labels Feb 27, 2025
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Thanks! Seems fine, though I didn't test. Can you provide a database I could import where this happens? Also, can you point to any open issue related to this? Also, please remove the parts of the template you don't use, when you open a PR.

@Isira-Seneviratne
Copy link
Member Author

Here's an example database: newpipe_subscriptions_202502280744.json

There isn't an open issue related to this I could find, it's just an issue I came across while running the app. I dug deeper and found that the stream uploader URL is different from the channel URL, so the notifications aren't properly grouped.

@Stypox
Copy link
Member

Stypox commented Feb 28, 2025

Mmmh, I still get a lot of distinct notifications, and there does not seem to be a difference between what I get with this PR and what I get before this PR.

Initial (**all** notifications are from TTaTST) After unsubscribing from TTaTST
Before After

I tested this code by deleting all feed data manually by putting this code somewhere:

        new Thread(
                () -> {
                    NewPipeDatabase.getInstance(requireContext()).feedDAO().deleteAll();
                    NewPipeDatabase.getInstance(requireContext()).feedGroupDAO().deleteAll();
                    NewPipeDatabase.getInstance(requireContext()).streamDAO().deleteAll();
                }
        ).start();

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

I tried logging all of the channel URLs being used for setGroup and they seem correct so it's just Android deciding not to group them for some reason, sorry for the wrong review. I don't understand how Android groups notifications, because I get a mess of notifications grouped badly, but oh well...

@Stypox Stypox merged commit ea20ca9 into TeamNewPipe:dev Feb 28, 2025
9 checks passed
@Isira-Seneviratne
Copy link
Member Author

I tried logging all of the channel URLs being used for setGroup and they seem correct so it's just Android deciding not to group them for some reason, sorry for the wrong review. I don't understand how Android groups notifications, because I get a mess of notifications grouped badly, but oh well...

https://developer.android.com/develop/ui/views/notifications/group

@Stypox
Copy link
Member

Stypox commented Feb 28, 2025

Automatic grouping behavior might vary on some device types. To provide the best experience on all devices and versions, if you know notifications must be grouped, specify a group key and group summary to make sure they are grouped.

So maybe we need to specify one notification to be the group summary for each channel?

@Isira-Seneviratne
Copy link
Member Author

So maybe we need to specify one notification to be the group summary for each channel?

That's already being done in line 60, the issue was that the group key was different for the summary notification and the other notifications.

@Stypox
Copy link
Member

Stypox commented Mar 5, 2025

I still wonder why they were not grouping well when I tested it (there were like 100s of ungrouped notifications from the same channel), but the code looks indeed correct

@Isira-Seneviratne Isira-Seneviratne deleted the Fix-notification-grouping branch April 10, 2025 06:06
@Stypox Stypox mentioned this pull request Jul 17, 2025
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Issue is related to a bug new stream notification Issue is related to notification shown for new streams size/small PRs with less than 50 changed lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants