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.

Sign up for GitHub

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

Help fragment migration #4174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+545 −162
Open

Help fragment migration #4174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e22d4bf
Migrated HelpFragment from XML to JetPackCompose Issue Id #4159 with …
SOUMEN-PAL Jan 14, 2025
250eb55
#4159 Migrated the HelpFragment to HelpScreen and changed the KiwixHe…
SOUMEN-PAL Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 1 addition & 20 deletions .idea/codeStyles/Project.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 43 additions & 5 deletions .idea/inspectionProfiles/kiwixAndroidInspections.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 26 additions & 0 deletions app/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import javax.xml.transform.stream.StreamResult
plugins {
android
id("com.github.triplet.play") version Versions.com_github_triplet_play_gradle_plugin
id("org.jetbrains.kotlin.android")
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have added this to our core module which is already included in app and custom modules. So why we are again adding this here?

id("org.jetbrains.kotlin.plugin.compose") version "2.1.0"
}
if (hasProperty("testingMinimizedBuild")) {
apply(plugin = "com.slack.keeper")
Expand Down Expand Up @@ -108,6 +110,13 @@ android {
// is analyzed to provide actionable feedback on potential issues with dependencies.
includeInBundle = true
}

buildFeatures {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for this?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing this out
Initially, I was testing the composable invocation in one module and faced some import-related issues with the other module, which led to this addition. However, I realize now that the business logic resides in the core module, and the issue can be resolved there directly. I'll fix this to avoid duplication and ensure consistency across the project.

compose = true
}
composeOptions {
kotlinCompilerExtensionVersion = "1.5.15"
}
}

play {
Expand All @@ -129,6 +138,23 @@ androidComponents {
dependencies {
androidTestImplementation(Libs.leakcanary_android_instrumentation)
testImplementation(Libs.kotlinx_coroutines_test)

implementation("androidx.compose.material3:material3-android:1.3.1")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dependencies should be included in one place, we are adding same dependencies here and as well as in core module.

The core module is the base for both app and custom apps so whatever is common for both modules we should include that in core module so that it can easily use in both apps. Or you can use the AllProjectConfigure class to declare the dependencies which is common for both modules.

implementation("androidx.activity:activity-compose:1.9.3")

implementation("androidx.compose.ui:ui:1.7.6")
implementation("androidx.compose.material:material:1.7.6")

implementation(platform("androidx.compose:compose-bom:2024.12.01"))
implementation("androidx.compose.ui:ui")
implementation("androidx.compose.ui:ui-tooling-preview")
implementation("androidx.compose.material:material")
implementation("androidx.compose.runtime:runtime-livedata")
implementation("androidx.compose.runtime:runtime-rxjava2")

// For testing
androidTestImplementation("androidx.compose.ui:ui-test-junit4:1.7.6")
debugImplementation("androidx.compose.ui:ui-tooling")
}
task("generateVersionCodeAndName") {
val file = File("VERSION_INFO")
Expand Down
78 changes: 65 additions & 13 deletions app/src/main/java/org/kiwix/kiwixmobile/help/KiwixHelpFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,77 @@

package org.kiwix.kiwixmobile.help

import org.kiwix.kiwixmobile.core.R
import org.kiwix.kiwixmobile.core.help.HelpFragment
import android.content.Context
import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.compose.ui.platform.ComposeView
import androidx.fragment.app.Fragment
import androidx.navigation.NavController
import androidx.navigation.Navigation
import org.kiwix.kiwixmobile.R
import org.kiwix.kiwixmobile.core.help.HelpScreen
import org.kiwix.kiwixmobile.core.help.HelpScreenItemDataClass
import org.kiwix.kiwixmobile.core.utils.SharedPreferenceUtil

class KiwixHelpFragment : HelpFragment() {
override fun rawTitleDescriptionMap() =
if (sharedPreferenceUtil.isPlayStoreBuildWithAndroid11OrAbove()) {
open class KiwixHelpFragment : Fragment() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SOUMEN-PAL Why are we directly extending the Fragment instead of HelpFragment? HelpFragment is the base for both apps module and we should use that for resusability.

Copy link
Author

Choose a reason for hiding this comment

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

The decision to extend Fragment directly instead of HelpFragment was intentional to align with Compose's architecture and avoid unnecessary coupling. HelpFragment is tightly tied to XML-based UI with responsibilities like managing a RecyclerView, viewBinding, and toolbar setup, which are better handled in Compose using LazyColumn and TopAppBar.

Using HelpFragment would introduce dependencies irrelevant to Compose and disrupt the clean separation Compose promotes. Extending Fragment allowed for a smoother integration of Compose while minimizing changes to the existing codebase.

Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@SOUMEN-PAL We have the same screen for the Custom app(which is missing in this PR so we are only converting the KiwixHelpFragment in compose), and HelpFragment is used as the base class for both screens. The only difference we are showing some additional things in Kiwix app so this is easily achievable by extending the existing HelpFragment. According to the current approach, we have to written the same code for customHelpFragment. Compose also offers reusability and we should make the code reusable. Clean separation is also achieved by extending the HelpFragment for both KiwixHelpFragment, and CustomHelpFragment as the HelpFragment is the base class that shares the common functionality for both screens and any additional things are written in the classes themselves.

HelpFragment is tightly tied to XML-based UI with responsibilities like managing a RecyclerView, viewBinding, and toolbar setup,

That's why we should convert the HelpFragment in the compose, and KiwixHelpFragment, and CustomHelpFragment will be unchanged(as there is no UI logic) and we will provide the additional data to the base class(HelpFragment) and it will render the UI for both app and custom apps.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I will do the required changes right away.
what I plan to do Is.
commit Back to prev recent stable heads and changing according to it.

private lateinit var navController: NavController
override fun onCreateView(
inflater: LayoutInflater,
container: ViewGroup?,
savedInstanceState: Bundle?
): View {
val context = requireContext()
val sharedPrefUtil = SharedPreferenceUtil(context)

val rawData = if (sharedPrefUtil.isPlayStoreBuildWithAndroid11OrAbove()) {
listOf(
R.string.help_2 to R.array.description_help_2,
R.string.help_5 to R.array.description_help_5,
R.string.how_to_update_content to R.array.update_content_description,
R.string.why_copy_move_files_to_app_directory to getString(
R.string.copy_move_files_to_app_directory_description
org.kiwix.kiwixmobile.core.R.string.help_2 to org.kiwix.kiwixmobile.core.R.array
.description_help_2,
org.kiwix.kiwixmobile.core.R.string.help_5 to org.kiwix.kiwixmobile.core.R.array
.description_help_5,
org.kiwix.kiwixmobile.core.R.string.how_to_update_content to org.kiwix.kiwixmobile
.core.R.array.update_content_description,
org.kiwix.kiwixmobile.core.R.string.why_copy_move_files_to_app_directory to getString(
org.kiwix.kiwixmobile.core.R.string.copy_move_files_to_app_directory_description
)
)
} else {
listOf(
R.string.help_2 to R.array.description_help_2,
R.string.help_5 to R.array.description_help_5,
R.string.how_to_update_content to R.array.update_content_description
org.kiwix.kiwixmobile.core.R.string.help_2 to org.kiwix.kiwixmobile.core.R.array
.description_help_2,
org.kiwix.kiwixmobile.core.R.string.help_5 to org.kiwix.kiwixmobile.core.R.array
.description_help_5,
org.kiwix.kiwixmobile.core.R.string.how_to_update_content to org.kiwix.kiwixmobile
.core.R.array.update_content_description
)
}

val helpScreenData: List<HelpScreenItemDataClass> = transformToHelpScreenData(context, rawData)
navController = Navigation.findNavController(requireActivity(), R.id.nav_host_fragment)

return ComposeView(requireContext()).apply {
setContent {
HelpScreen(data = helpScreenData, navController = navController)
}
}
}
}

fun transformToHelpScreenData(
context: Context,
rawTitleDescriptionMap: List<Pair<Int, Any>>
): List<HelpScreenItemDataClass> {
return rawTitleDescriptionMap.map { (titleResId, description) ->
val title = context.getString(titleResId)
val descriptionValue = when (description) {
is String -> description
is Int -> context.resources.getStringArray(description).joinToString(separator = "\n")
else -> {
throw IllegalArgumentException("Invalid description resource type for title: $titleResId")
}
}
HelpScreenItemDataClass(title, descriptionValue)
}
}
26 changes: 26 additions & 0 deletions core/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ buildscript {
}
plugins {
`android-library`
id("org.jetbrains.kotlin.android")
id("org.jetbrains.kotlin.plugin.compose") version "2.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do no hardcode version number here, We are managing all the dependencies in Libs and Versions class, so define all the dependencies related things there.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the information I had experience with TOML files thats why I confused my self I will fix them right away.

}
plugins.apply(KiwixConfigurationPlugin::class)
apply(plugin = "io.objectbox")
Expand All @@ -26,6 +28,12 @@ android {
isMinifyEnabled = false
}
}
buildFeatures {
compose = true
}
composeOptions {
kotlinCompilerExtensionVersion = "1.5.15"
}
}

fun shouldUseLocalVersion() = File(projectDir, "libs").exists()
Expand Down Expand Up @@ -63,4 +71,22 @@ dependencies {
implementation(Libs.kotlinx_coroutines_android)
implementation(Libs.kotlinx_coroutines_rx3)
implementation(Libs.zxing)

// Compose ans Material3 Dependencies
implementation("androidx.compose.material3:material3-android:1.3.1")
implementation("androidx.activity:activity-compose:1.9.3")

implementation("androidx.compose.ui:ui:1.7.6")
implementation("androidx.compose.material:material:1.7.6")

implementation(platform("androidx.compose:compose-bom:2024.12.01"))
implementation("androidx.compose.ui:ui")
implementation("androidx.compose.ui:ui-tooling-preview")
implementation("androidx.compose.material:material")
implementation("androidx.compose.runtime:runtime-livedata")
implementation("androidx.compose.runtime:runtime-rxjava2")

// For testing
androidTestImplementation("androidx.compose.ui:ui-test-junit4:1.7.6")
debugImplementation("androidx.compose.ui:ui-tooling")
}
3 changes: 1 addition & 2 deletions core/objectbox-models/default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"_note1": "KEEP THIS FILE! Check it into a version control system (VCS) like git.",
"_note2": "ObjectBox manages crucial IDs for your object model. See docs for details.",
"_note3": "If you have VCS merge conflicts, you must resolve them according to ObjectBox docs.",
"_note4": "The IDs of the entities and properties should not be changed unless there is a change in the corresponding entity.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we changing the json file?

Copy link
Author

Choose a reason for hiding this comment

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

due to some auto imports done by me using Alt+Enter i guess this changes had happen by me by mistake i will fix them right now.
Sorry for the unintended problems.

"entities": [
{
"id": "3:5536749840871435068",
Expand Down Expand Up @@ -403,4 +402,4 @@
],
"retiredRelationUids": [],
"version": 1
}
}
Loading
Loading