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

New streams notifications #2335

Merged
merged 57 commits into from
Mar 20, 2022
Merged

Conversation

Koitharu
Copy link
Contributor

@Koitharu Koitharu commented May 9, 2019

Checking and notifying about new streams from subscriptions
Closes #719 closes #1086

Screenshot_2019-05-09-21-00-19 Screenshot_2019-05-09-21-02-27

@Redirion
Copy link
Member

cool!
a nice addition to that feature would be to be able to select the channels for which I want to receive notifications similar to the "bell" feature in Youtube.

@Koitharu
Copy link
Contributor Author

Ok, I'll do it. Also I have a question to the community: how is better to mark, which streams are already "known" at channel?

@Stypox
Copy link
Member

Stypox commented May 10, 2019

I think a video should be marked as "known" after it has been opened (in any way: background, info, popup...). An option to manually mark a video as known would be useful, too, then.

@Koitharu Koitharu marked this pull request as ready for review June 30, 2019 13:11
@theScrabi theScrabi added the feature request Issue is related to a feature in the app label Jul 21, 2019
@Bugsbane

This comment has been minimized.

@Stypox Stypox mentioned this pull request Sep 8, 2019
3 tasks
@Stypox Stypox mentioned this pull request Dec 28, 2019
@ShareASmile
Copy link
Collaborator

ShareASmile commented Mar 29, 2020

Ahh.. Waiting for live stream notifications for so long..

Edit:
Now as proper feed implementation by
#2309
has already taken place, this new stream notification should be next big feature to make into this marvellous app The NewPipe.

@Stypox Stypox linked an issue Mar 30, 2020 that may be closed by this pull request
@Flam3z
Copy link

Flam3z commented Apr 2, 2020

Definitely would love to see this feature as the next big thing as well :)

@Stypox Stypox added the feed Issue is related to the feed label Apr 10, 2020
@Koitharu
Copy link
Contributor Author

What about merging this PR? I`ll resolve conflicts and migrate to androidx WorkManager

@wb9688
Copy link
Contributor

wb9688 commented Apr 12, 2020

@nv95: That's good. Please use vector drawables for the new icons though (and test that it doesn't cause a crash on Android API 19). Note that there were some major changes to the subscriptions in v0.19.0.

Imho the notifications should be opt-in though.

@Koitharu Koitharu force-pushed the feature/notifications branch 2 times, most recently from 112b084 to e396cb8 Compare April 18, 2020 11:06
@Koitharu

This comment has been minimized.

@Stypox Stypox modified the milestones: 0.20.0, 0.19.4 Apr 20, 2020
@B0pol
Copy link
Member

B0pol commented May 10, 2020

@nv95: That's good. Please use vector drawables for the new icons though (and test that it doesn't cause a crash on Android API 19). Note that there were some major changes to the subscriptions in v0.19.0.

Imho the notifications should be opt-in though.

I don't know if it was the case but it's now opt-in

B0pol
B0pol previously requested changes May 10, 2020
Copy link
Member

@B0pol B0pol left a comment

Choose a reason for hiding this comment

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

I've seen you have to subscribe to notifications via settings -> notification -> channels

Please add the notification button in channel header too, it's way more user-friendly.

And add a subscribe to all button in notification settings.

@B0pol
Copy link
Member

B0pol commented May 10, 2020

Also, why is it leading to channel info instead of video info?

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.

Some icons unfortunately can't be converted to vector drawables because they are used in notifications, which require bitmaps. But there are still some issues with icons.

Btw @wb9688 how were you able to rebase this while being sure not to create problems even in mid commits? I tried a while ago, but there were so many conflicting commits that trying to understand what the author was doing in each one of them (i.e. the purpose of every piece of code) seemed impossible

@@ -532,4 +591,10 @@ public void setTitle(final String title) {
headerTitleView.setText(title);
}
}

private boolean isNotificationsEnabled() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to put this in the NotificationHelper class, so that is can then be accessed from anywhere. In the future we could e.g. want to long-press on a subscription to see the option for notifications and this function would be needed.

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
Comment on lines 48 to 61
final NotificationCompat.Builder builder = new NotificationCompat.Builder(context,
context.getString(R.string.streams_notification_channel_id));
builder.setContentTitle(String.format("%s • %s", data.getName(), summary));
builder.setContentText(data.getText());
builder.setNumber(data.getSize());
builder.setBadgeIconType(NotificationCompat.BADGE_ICON_LARGE);
builder.setPriority(NotificationCompat.PRIORITY_DEFAULT);
builder.setSmallIcon(R.drawable.ic_stat_newpipe);
builder.setLargeIcon(BitmapFactory.decodeResource(context.getResources(),
R.drawable.ic_newpipe_triangle_white));
builder.setColor(ContextCompat.getColor(context, R.color.ic_launcher_background));
builder.setColorized(true);
builder.setAutoCancel(true);
builder.setCategory(NotificationCompat.CATEGORY_SOCIAL);
Copy link
Member

Choose a reason for hiding this comment

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

If this is a Builder, can't you do this? Also, maybe create the style beforehand, so that you can add the .setStyle() and .setContentIntent() at the end

Suggested change
final NotificationCompat.Builder builder = new NotificationCompat.Builder(context,
context.getString(R.string.streams_notification_channel_id));
builder.setContentTitle(String.format("%s • %s", data.getName(), summary));
builder.setContentText(data.getText());
builder.setNumber(data.getSize());
builder.setBadgeIconType(NotificationCompat.BADGE_ICON_LARGE);
builder.setPriority(NotificationCompat.PRIORITY_DEFAULT);
builder.setSmallIcon(R.drawable.ic_stat_newpipe);
builder.setLargeIcon(BitmapFactory.decodeResource(context.getResources(),
R.drawable.ic_newpipe_triangle_white));
builder.setColor(ContextCompat.getColor(context, R.color.ic_launcher_background));
builder.setColorized(true);
builder.setAutoCancel(true);
builder.setCategory(NotificationCompat.CATEGORY_SOCIAL);
final NotificationCompat.Builder builder = new NotificationCompat.Builder(context,
context.getString(R.string.streams_notification_channel_id))
.setContentTitle(String.format("%s • %s", data.getName(), summary))
.setContentText(data.getText())
.setNumber(data.getSize())
.setBadgeIconType(NotificationCompat.BADGE_ICON_LARGE)
.setPriority(NotificationCompat.PRIORITY_DEFAULT)
.setSmallIcon(R.drawable.ic_stat_newpipe)
.setLargeIcon(BitmapFactory.decodeResource(context.getResources(),
R.drawable.ic_newpipe_triangle_white))
.setColor(ContextCompat.getColor(context, R.color.ic_launcher_background))
.setColorized(true)
.setAutoCancel(true)
.setCategory(NotificationCompat.CATEGORY_SOCIAL);

builder.setCategory(NotificationCompat.CATEGORY_SOCIAL);
final NotificationCompat.InboxStyle style = new NotificationCompat.InboxStyle();
for (StreamInfoItem o : data.getStreams()) {
style.addLine(o.getName());
Copy link
Member

Choose a reason for hiding this comment

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

Question: why are stream names shown both in builder.setContentText(data.getText()); and here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

builder.setContentText is used for collapsed notification, style applied for expanded

}

override fun onSharedPreferenceChanged(sharedPreferences: SharedPreferences?, key: String?) {
val ctx = context ?: return
Copy link
Member

Choose a reason for hiding this comment

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

This usage of ctx looks a bit strange, though I don't know if this piece of code can be reworded


private fun updateSubscriptions(list: List<SubscriptionEntity>) {
var notified = 0
for (o in list) {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling it o and list, I think subscription and subscriptions would be better names


fun update(newData: List<SubscriptionEntity>?) {
dataSet.clear()
dataSet.addAll(newData!!)
Copy link
Member

Choose a reason for hiding this comment

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

Why make it nullable and then assert it is not null?

Comment on lines 76 to 86
val icon = AnimatedVectorDrawableCompat.create(
itemView.context,
if (mode == NotificationMode.DISABLED) {
R.drawable.av_notification_off
} else {
R.drawable.av_notification_on
}
) ?: return null
icon.setTint(ThemeHelper.resolveColorFromAttr(
itemView.context, android.R.attr.textColorPrimary
))
Copy link
Member

Choose a reason for hiding this comment

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

You should use an attr icon here, so that it can be styled according to themes automatically. Also, why is the name of those icons not ic_notification_on_white_24dp? So, in the end you should have 4 icons (ic_notification_on_white_24dp, ic_notification_on_black_24dp, ic_notification_off_white_24dp, ic_notification_off_black_24dp)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What reasons to use different icons if we can use one icon with tint?

@Stypox Stypox removed this from the 0.19.4 milestone May 28, 2020
@TobiGr TobiGr requested review from litetex and removed request for AudricV February 21, 2022 20:37
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 pushed a commit making some small changes. The code looks good, now it just needs testing. I will be leaving notifications on on my phone with my subscriptions database for a while.

@Stypox
Copy link
Member

Stypox commented Feb 25, 2022

 I will be leaving notifications on on my phone with my subscribers database for a while.

After two days everything still seems to be up and running

Removed some non-translatable strings and just hardcoded them in the code, like it's being done for other string separators. This also deduplicates some code by using Localization.
Used some Kotlin feature to reduce code.
@Stypox Stypox force-pushed the feature/notifications branch from 8ffcb16 to 5fea12d Compare February 26, 2022 09:40
@litetex litetex removed their request for review March 2, 2022 19:36
Stypox added 2 commits March 19, 2022 21:55
It never kicked in since we are never returning a retry() Result, but always either success() or failure() (see createWork() function). Also, there is already a default (exponential backoff starting from 30 seconds), so no need to override it.
@Stypox Stypox force-pushed the feature/notifications branch from cb83d33 to 6e8c9f9 Compare March 19, 2022 21:29
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

  • Merged and fixed conflicts
  • Added a commit removing the backoff criteria completely
  • Added a commit that makes the "Player notification" entry searchable in settings. I don't mean the settings contained within player notification, as those were already searchable, but rather just the button in appearance settings that brings you to player notification settings: it is now searchable.
  • I tested for a few days on my phone (a couple of weeks ago) and notifications worked fine.
  • I tested again now on the API 19 and API 30 emulators and notifications worked. Also other things this PR touched worked, such as settings, subscribing to channels, feed, etc.
  • Code was reviewed by all of us and now looks good.

I would merge this, finally :-D

Note for after this is merged: create a PR that removes both the ic_notifications and ic_pin icons in the drawable-night folders, as the way icons work has been changed. I will not do this in this PR since ic_pin is unrelated.

@Stypox Stypox merged commit 2623f0e into TeamNewPipe:dev Mar 20, 2022
@Stypox
Copy link
Member

Stypox commented Mar 20, 2022

Update Database Wiki page

@TobiGr or @litetex could you take care of that? I don't know what should be updated (just the screenshot of an inspected database?)

@litetex
Copy link
Member

litetex commented Mar 20, 2022

@Stypox @TobiGr
Not sure how the graphic is built is there a manual somewhere?

@Stypox
Copy link
Member

Stypox commented Mar 20, 2022

I think you can view it if you inspect the app's database of a running app, but I never did that

@Stypox
Copy link
Member

Stypox commented Mar 26, 2022

@TiA4f8R could you point out here the issues you found with new streams notifications?

@AudricV
Copy link
Member

AudricV commented Mar 26, 2022

These are not really issues but improvements (I didn't test deeply the feature):

  • show the avatar of the channel when selecting channels to recieve subscriptions;
  • in the notifications, be able to click on a specific stream to open it instead of being redirected to the channel itself;
  • (maybe) show a bell icon to enable/disable notifications at the right of the subscribe button, like on YouTube, instead of showing a snackbar.

@litetex
Copy link
Member

litetex commented Mar 26, 2022

@Stypox

I created the follwoing database-diagramm with https://dbeaver.io/ (FOSS):

Feel free to update the wiki if you feel like this is correct.

@Stypox
Copy link
Member

Stypox commented Mar 27, 2022

@litetex done, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app feed Issue is related to the feed
Projects
None yet