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

History cache compose #11494

Open
wants to merge 30 commits into
base: refactor
Choose a base branch
from

Conversation

braiso-22
Copy link

@braiso-22 braiso-22 commented Sep 1, 2024

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

  • Added daggerHilt DI
  • Changed the History and cache setting screen to use compose

Before/After Screenshots/Screen Record

  • Before:
    Captura de pantalla 2024-09-01 a las 18 44 50
    image

  • After:
    Captura de pantalla 2024-09-01 a las 18 39 49
    image

Fixes the following issue(s)

  • Fixes #

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

theScrabi and others added 29 commits July 2, 2024 09:31
add NP icon for Android Studio's NewUI
…-npe

Fix crash in MediaSessionPlayerUi while destroying player
* Update Matrix room link, and prioritise it

* Update Matrix room link in CONTRIBUTING.md

* Prioritise Matrix in contribution doc too
Likely related to mozilla/rhino@01a7b20 but I am not completely sure.
I tested the app and it works well, so I think that org.mozilla.javascript.JavaToJSONConverters is not used really.

This is the full list of errors:
Missing class java.beans.BeanDescriptor (referenced from: java.lang.Object org.mozilla.javascript.JavaToJSONConverters.lambda$static$4(java.lang.Object))
Missing class java.beans.BeanInfo (referenced from: java.lang.Object org.mozilla.javascript.JavaToJSONConverters.lambda$static$4(java.lang.Object))
Missing class java.beans.IntrospectionException (referenced from: java.lang.Object org.mozilla.javascript.JavaToJSONConverters.lambda$static$4(java.lang.Object))
Missing class java.beans.Introspector (referenced from: java.lang.Object org.mozilla.javascript.JavaToJSONConverters.lambda$static$4(java.lang.Object))
Missing class java.beans.PropertyDescriptor (referenced from: java.lang.Object org.mozilla.javascript.JavaToJSONConverters.lambda$static$4(java.lang.Object))
returnActivity was removed in 463dd8e
…y-test

Remove outdated returnActivity test code
…in multiple README.md's translated into different languages (TeamNewPipe#11487)

Link to contribution notes wasn't working

* Update README.de.md, fix grammar in README.de.md
* Update README.asm.md
* Update README.fr.md
* Update README.hi.md
* Update README.it.md
* Update README.pa.md
* Update README.pt_BR.md
* Update README.ru.md
* Update README.sr.md

---------

Co-authored-by: Tobi <[email protected]>
@github-actions github-actions bot added the size/giant PRs with more than 750 changed lines label Sep 1, 2024
@braiso-22
Copy link
Author

@snaik20 can you review this PR?

@TobiGr TobiGr added the GUI Issue is related to the graphical user interface label Sep 2, 2024
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne left a comment

Choose a reason for hiding this comment

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

This looks pretty decent, thank you! In particular, the addition of Hilt will be useful.

That being said, I've suggested some improvements.

Comment on lines +8 to +18
class OpenErrorActivity(
private val context: Context,
) {
operator fun invoke(errorInfo: ErrorInfo) {
val intent = Intent(context, ErrorActivity::class.java)
intent.putExtra(ErrorActivity.ERROR_INFO, errorInfo)
intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK)

context.startActivity(intent)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This would be better as a static method in the error activity class itself.

Comment on lines +31 to +37
val clickModifier = if (enabled) {
Modifier.clickable { onClick() }
} else {
Modifier
}
Row(
modifier = clickModifier.then(modifier),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val clickModifier = if (enabled) {
Modifier.clickable { onClick() }
} else {
Modifier
}
Row(
modifier = clickModifier.then(modifier),
Row(
modifier = Modifier
.clickable(enabled, onClick = onClick)
.then(modifier),

Comment on lines +22 to +31
) = ComposeView(requireContext()).apply {
setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed)
setContent {
AppTheme {
HistoryCacheSettingsScreen(
modifier = Modifier.fillMaxSize()
)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
) = ComposeView(requireContext()).apply {
setViewCompositionStrategy(ViewCompositionStrategy.DisposeOnViewTreeLifecycleDestroyed)
setContent {
AppTheme {
HistoryCacheSettingsScreen(
modifier = Modifier.fillMaxSize()
)
}
}
}
) = content {
AppTheme {
HistoryCacheSettingsScreen(modifier = Modifier.fillMaxSize())
}
}

This requires the Fragment Compose dependency.

Comment on lines +1 to +7
package org.schabi.newpipe.settings.domain.usecases.get_preference

import kotlinx.coroutines.flow.Flow

fun interface GetPreference<T> {
operator fun invoke(key: Int, defaultValue: T): Flow<T>
}
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne Sep 2, 2024

Choose a reason for hiding this comment

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

Using DataStore might be a better option instead of adding custom classes for handling preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issue is related to the graphical user interface size/giant PRs with more than 750 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants