Refactor generating InfoItemDialog's #7570

TobiGr · 2021-12-22T19:58:39Z

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

I stumbled on this while doing some other code improvements and did not like the old structure for a few reasons. Most importantly, separation of concerns was neglected and the code was not easy to understand, long and full of duplicate lines.

This PR refactors the way InfoItemDialogs are generated. This is necessary because the old way used the StreamDialogEntry enum for most of the dialogs' content generation process. This required static variables and methods to store the entries which are used for the dialog to be build (See e.g.enabledEntries and methods like generateCommands()). In other words, StreamDialogEntry wasn't an enumeration anymore.

To address this issue, a Builder is introduced for the InfoItemDialog's genration.

A second problem that introduced a structure which was atypical for an enumeration was the usage of non-final attributes within StreamDialogEntry instances. These were needed because the default actions needed to be overriden in some cases.

To address this problem, the StreamDialogEntry enumeration was renamed to StreamDialogDefaultEntry and a new StreamDialogEntry class is used instead.

ToDo

Add a few more comments
Use consistent names for methods that create the dialogs within the Fragments.
Reduce code duplication. Some parts have the same logic which decides which entries are added to the dialog. Maybe we can find a way to generalize this and extract those lines to the Builder.
Check why InfoItemDialog does not extend AlertDialog, but contains one.
Answer: Extending AlertDialog and creating the contentView is a pain.
squash commits

Note

I am not happy that we have StreamDialogEntry and StreamDialogDefaultEntry. However, I am not sure if there is a better solution.

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.

Due diligence

I read the contribution guidelines.

sonarqubecloud · 2021-12-27T15:45:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

litetex

LGTM, some minor things I found while doing a quick review:

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java

litetex · 2022-01-01T15:06:53Z

Did a quick test (searched something and clicked through the long-press dropdown) and almost everything seems to work as expected.
However when I try to mark a video as watched it isn't marked as watched immediately and you have to search the same term again. Not sure if this was so before but I think this should be fixed (maybe in a new issue/PR if it was so before).

Stypox · 2022-01-07T20:22:00Z

However when I try to mark a video as watched it isn't marked as watched immediately and you have to search the same term again. Not sure if this was so before but I think this should be fixed (maybe in a new issue/PR if it was so before).

~~This never worked for search lists, and after (I think) #7050 it stopped working in the feed fragment, too. Could you look into it?~~ Edit: see #7545

Stypox

Looks mostly good, thank you for this improvement :-)

In other words, StreamDialogEntry wasn't an enumeration anymore.

Maybe the DefaultStreamDialogEntry enum should be removed altogether and its entries should be moved to StreamDialogEntry as public static final StreamDialogEntry fields?

app/src/main/java/org/schabi/newpipe/fragments/list/BaseListFragment.java

Stypox · 2022-01-27T16:30:21Z

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java

+     * Custom actions for entries can be set using
+     * {@link #setAction(StreamDialogDefaultEntry, StreamDialogEntry.StreamDialogEntryAction)}.
+     */
+    public static class Builder {


In the Javadoc I'd explain that it can only be used to show a dialog for StreamInfoItems, not just any InfoItem

I thought about renaming the whole class to StreamInfoItemDialog to make it clear that the dialog only works with StreamInfoItems currently.
On the other hand, we could create more builders to match the missing InfoItems: channels and playlists. Their held() methods are not overridden in many cases.

app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java

app/src/main/java/org/schabi/newpipe/util/StreamDialogDefaultEntry.java

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java

TobiGr · 2022-02-18T22:56:01Z

I rebased this PR. However, I screwed up while doing so. As a result my new commits are trash. Sorry for that.

Stypox

Almost ready

app/src/main/java/org/schabi/newpipe/info_list/dialog/StreamDialogEntry.java

This commit refactors the way `InfoItemDialog`s are generated. This is necessary because the old way used the `StreamDialogEntry` enum for most of the dialogs' content generation process. This required static variables and methods to store the entries which are used for the dialog to be build (See e.g.`enabledEntries` and methods like `generateCommands()`). In other words, `StreamDialogEntry` wasn't an enumeration anymore. To address this issue, a `Builder` is introduced for the `InfoItemDialog`'s genration. The builder also comes with some default entries and and a specific order. Both can be used, but are not enforced. A second problem that introduced a structure which was atypical for an enumeration was the usage of non-final attributes within `StreamDialogEntry` instances. These were needed, because the default actions needed to overriden in some cases. To address this problem, the `StreamDialogEntry` enumeration was renamed to `StreamDialogDefaultEntry` and a new `StreamDialogEntry` class is used instead.

Improve docs

…nnel details

Return this in InfoIrtemDialog.Builder methoods. Move null checks for InfoIrtemDialog.Builder into constructor. Fix and add some more docs.

sonarqubecloud · 2022-02-21T20:59:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

TobiGr added the codequality Improvements to the codebase to improve the code quality label Dec 22, 2021

TobiGr force-pushed the improvement/infoItemDialogBuilder branch 4 times, most recently from 0abd88f to dbc7218 Compare December 27, 2021 15:34

TobiGr marked this pull request as ready for review December 27, 2021 15:46

litetex reviewed Jan 1, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java Outdated Show resolved Hide resolved

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java Outdated Show resolved Hide resolved

Stypox requested changes Jan 27, 2022

View reviewed changes

Stypox mentioned this pull request Jan 27, 2022

Fix crash when long-pressing stream while player is starting #7704

Merged

5 tasks

litetex reviewed Jan 28, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/util/StreamDialogDefaultEntry.java Outdated Show resolved Hide resolved

litetex reviewed Jan 28, 2022

View reviewed changes

app/src/main/java/org/schabi/newpipe/info_list/InfoItemDialog.java Outdated Show resolved Hide resolved

TobiGr mentioned this pull request Feb 4, 2022

#7036: Fix codestyle issues #7808

Closed

5 tasks

TobiGr force-pushed the improvement/infoItemDialogBuilder branch from dbc7218 to e5481ba Compare February 18, 2022 22:48

TobiGr force-pushed the improvement/infoItemDialogBuilder branch from e5481ba to 7c97dc1 Compare February 18, 2022 23:03

Stypox requested changes Feb 19, 2022

View reviewed changes

TobiGr added 9 commits February 20, 2022 20:17

Extract entries into beginning and end category

1cd3ef5

Add default entries automatically

50e2385

Use Context instead of Activity

962fe9c

Improve docs

Use ErrorActivity to notifiy about errors occourred while loading cha…

ef0d562

…nnel details

Use identical method names for creating the InfoItemDialog in Fragments

646d8f4

Apply feedback

fd0d76e

Return this in InfoIrtemDialog.Builder methoods. Move null checks for InfoIrtemDialog.Builder into constructor. Fix and add some more docs.

Fix rebase

a7d5d9a

Move Classes related to InfoItemDIalog into own package

277f21d

TobiGr force-pushed the improvement/infoItemDialogBuilder branch from 7c97dc1 to ba0f0bc Compare February 20, 2022 19:22

Move StreamDialogEntry.openChannelFragment to NavigationHelper

ee477b2

TobiGr force-pushed the improvement/infoItemDialogBuilder branch from ba0f0bc to ee477b2 Compare February 20, 2022 19:26

Clarify that only StramInfoItems are accepted by the builder

d3bc184

This was referenced Feb 24, 2022

Duplicated methods that fetch full info for sparse streams #7941

Closed

Create an app-wide Disposable to be used for app-wide actions #7942

Closed

Stypox approved these changes Feb 24, 2022

View reviewed changes

litetex merged commit 37517c7 into dev Feb 26, 2022

litetex deleted the improvement/infoItemDialogBuilder branch February 26, 2022 15:18

Stypox mentioned this pull request Apr 16, 2022

Release 0.23.0 (please TEST!) #8230

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor generating InfoItemDialog's #7570

Refactor generating InfoItemDialog's #7570

TobiGr commented Dec 22, 2021 •

edited

Loading

sonarqubecloud bot commented Dec 27, 2021

litetex left a comment

litetex commented Jan 1, 2022

Stypox commented Jan 7, 2022 •

edited

Loading

Stypox left a comment •

edited

Loading

Stypox Jan 27, 2022

TobiGr Jan 28, 2022

TobiGr commented Feb 18, 2022

Stypox left a comment

sonarqubecloud bot commented Feb 21, 2022

Refactor generating InfoItemDialog's #7570

Refactor generating InfoItemDialog's #7570

Conversation

TobiGr commented Dec 22, 2021 • edited Loading

What is it?

Description of the changes in your PR

ToDo

Note

APK testing

Due diligence

sonarqubecloud bot commented Dec 27, 2021

litetex left a comment

Choose a reason for hiding this comment

litetex commented Jan 1, 2022

Stypox commented Jan 7, 2022 • edited Loading

Stypox left a comment • edited Loading

Choose a reason for hiding this comment

Stypox Jan 27, 2022

Choose a reason for hiding this comment

TobiGr Jan 28, 2022

Choose a reason for hiding this comment

TobiGr commented Feb 18, 2022

Stypox left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 21, 2022

TobiGr commented Dec 22, 2021 •

edited

Loading

Stypox commented Jan 7, 2022 •

edited

Loading

Stypox left a comment •

edited

Loading