Help fragment migration #4174

SOUMEN-PAL · 2025-01-15T08:39:38Z

###Changes Made

Added necessary Jetpack Compose dependencies and plugins to both the app module and core module build.gradle.kts files.
Created two composable Kotlin files:
- HelpScreen.kt: Contains the UI for the HelpFragment using LazyColumn and AnimatedVisibility.
- HelpScreenItem.kt: Handles individual items within the HelpScreen UI.
Created a data class HelpScreenItemDataClass (serving as a replacement for the HelpItem class in the previous HelpAdapter) to define the required data items and strings for the HelpScreen.
Updated KiwixHelpFragment.kt to make it compatible with Jetpack Compose by:
- Returning a ComposeView instead of inflating XML layouts.
- Changing its inheritance from HelpFragment to the default Fragment to enable calling composable functions directly.
Fetched the existing navGraph and integrated it with a new navController to support seamless navigation between XML-based UI and Compose-based UI.

Screenshots

Earlier vs New UI

Earlier UI in XML	New Migrated UI in JC (Jetpack Compose)

Demo Video

MigrationHelp.mp4

…with lint checks off

…wixHelpFragment in app module to run Compossables, with addition of Jetpack compose compatibility in gradel app/core

MohitMaliFtechiz

@SOUMEN-PAL Thanks for the PR. See the review changes.

MohitMaliFtechiz · 2025-01-16T06:54:54Z

.idea/codeStyles/Project.xml

@@ -1,7 +1,5 @@
 <component name="ProjectCodeStyleConfiguration">
  <code_scheme name="Project" version="173">
-    <option name="USE_SAME_INDENTS" value="true" />


@SOUMEN-PAL Please do not include automatic changes in the PR.

MohitMaliFtechiz · 2025-01-16T06:55:14Z

.idea/inspectionProfiles/kiwixAndroidInspections.xml

@@ -51,7 +51,6 @@
    <inspection_tool class="AndroidLintExportedPreferenceActivity" enabled="true" level="ERROR" enabled_by_default="true" />
    <inspection_tool class="AndroidLintExportedReceiver" enabled="true" level="ERROR" enabled_by_default="true" />
    <inspection_tool class="AndroidLintExportedService" enabled="true" level="ERROR" enabled_by_default="true" />
-    <inspection_tool class="AndroidLintExtraText" enabled="true" level="ERROR" enabled_by_default="true" />


Revert this as well.

MohitMaliFtechiz · 2025-01-16T06:56:33Z

core/src/main/res/values-b+be+tarask+old/strings.xml

@@ -15,39 +11,56 @@
  <string name="menu_read_aloud">Чытаць уголас</string>
  <string name="menu_read_aloud_stop">Спыніць чытаньне ўголас</string>
  <string name="menu_support_kiwix">Падтрымаць Kiwix</string>
+  <string name="menu_support_kiwix_for_custom_apps">Падтрымка %s</string>


Revert this as well.

MohitMaliFtechiz · 2025-01-16T06:57:12Z

core/objectbox-models/default.json

@@ -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.",


Why are we changing the json file?

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.

MohitMaliFtechiz · 2025-01-16T07:00:39Z

app/build.gradle.kts

@@ -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")


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?

MohitMaliFtechiz · 2025-01-16T07:03:17Z

app/build.gradle.kts

@@ -108,6 +110,13 @@ android {
    // is analyzed to provide actionable feedback on potential issues with dependencies.
    includeInBundle = true
  }
+
+  buildFeatures {


Same for this?

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.

MohitMaliFtechiz · 2025-01-16T07:10:56Z

app/build.gradle.kts

@@ -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")


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.

MohitMaliFtechiz · 2025-01-16T07:12:48Z

core/build.gradle.kts

@@ -13,6 +13,8 @@ buildscript {
 }
 plugins {
  `android-library`
+  id("org.jetbrains.kotlin.android")
+  id("org.jetbrains.kotlin.plugin.compose") version "2.1.0"


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.

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

MohitMaliFtechiz · 2025-01-16T07:44:27Z

app/src/main/java/org/kiwix/kiwixmobile/help/KiwixHelpFragment.kt

-class KiwixHelpFragment : HelpFragment() {
-  override fun rawTitleDescriptionMap() =
-    if (sharedPreferenceUtil.isPlayStoreBuildWithAndroid11OrAbove()) {
+open class KiwixHelpFragment : Fragment() {


@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.

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.

@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.

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.

MohitMaliFtechiz · 2025-01-16T09:14:47Z

core/src/main/java/org/kiwix/kiwixmobile/core/help/HelpScreen.kt

+        .padding(16.dp)
+    )
+    Text(
+      text = "Send Diagnostic Data",


Please do not hardcode text it prevents translation into other languages. Additionally, it is "Send diagnostic report" not "Send Diagnostic Data".

ok I would be fetching it from the main strings file and do the changes accordingly.

MohitMaliFtechiz · 2025-01-16T09:26:57Z

@SOUMEN-PAL For the lint issue, these are the detekt issues since the project is now starting supporting the compose so the detekt rules were not configured according to compose. Here is the doc of detekt https://detekt.dev/docs/introduction/compose/ how we can configure the rules of detekt for compose.

SOUMEN-PAL added 2 commits January 15, 2025 00:35

Migrated HelpFragment from XML to JetPackCompose Issue Id kiwix#4159 …

e22d4bf

…with lint checks off

kiwix#4159 Migrated the HelpFragment to HelpScreen and changed the Ki…

250eb55

…wixHelpFragment in app module to run Compossables, with addition of Jetpack compose compatibility in gradel app/core

kelson42 requested a review from MohitMaliFtechiz January 16, 2025 04:35

MohitMaliFtechiz requested changes Jan 16, 2025

View reviewed changes

MohitMaliFtechiz reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Help fragment migration #4174

Help fragment migration #4174

SOUMEN-PAL commented Jan 15, 2025

MohitMaliFtechiz left a comment

MohitMaliFtechiz Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

SOUMEN-PAL Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

SOUMEN-PAL Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

SOUMEN-PAL Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

SOUMEN-PAL Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025 •

edited

Loading

SOUMEN-PAL Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025

SOUMEN-PAL Jan 16, 2025

MohitMaliFtechiz commented Jan 16, 2025

Help fragment migration #4174

Are you sure you want to change the base?

Help fragment migration #4174

Conversation

SOUMEN-PAL commented Jan 15, 2025

Earlier vs New UI

MohitMaliFtechiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohitMaliFtechiz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MohitMaliFtechiz commented Jan 16, 2025

MohitMaliFtechiz Jan 16, 2025 •

edited

Loading