[Feature Request]: Migrate keyboard hiding functionality #5406
Labels
enhancement
End user-perceivable enhancements.
good first issue
This item is good for new contributors to make their pull request.
Impact: Low
Low perceived user impact (e.g. edge cases).
Work: Low
Solution is clear and broken into good-first-issue-sized chunks.
Comments
6 tasks
|
Hi, I would like to work on this issue. If no one has picked it yet, can you assign it to me? |
|
Hi @oyeraghib, assigning the issue to you. Feel free to raise a PR. |
theMr17
added
good first issue
BenHenning
added a commit
that referenced
this issue
Jun 3, 2024
…5402) ## Explanation Fixes part of #4120 Fixes part of #1051 Similar to #5400, this brings forward changes that would otherwise go in #4937 to simplify the transition to Kotlin 1.6. Part of #4937 is introducing warnings-as-errors for both Kotlin and Java in order to reduce developer error, simplify the codebase, and minimize warnings when building (which can result in developer habits of ignoring warnings that might have real consequences to end users of the app). In order to keep the main migration PR smaller, this PR fixes all existing warnings and any new ones detected with the Kotlin 1.6 compiler that are not tied to Kotlin 1.5/1.6 API changes (those are part of #4937, instead). Fortunately, most of the changes could be brought forward into this PR. Specific things to note: - A few new issues were filed for SDK 33 deprecations caused, but not noted by, #5222): #5404, #5405, and #5406 and corresponding TODOs added. This PR opts for TODOs over actual fixes to minimize the amount of manual verification needed, and to try and keep the PR more focused on non-functional refactor changes (to reduce the risk as reverting this PR may be difficult if an issue is introduced). - A lot of the fixes were removing redundant casts or null checks. - The old mechanism we used for view models is deprecated, and had a lot of problems (partially documented in #1051). This PR moves the codebase over to directly injecting view models instead of using the view model provider (thus getting rid of true Jetpack view models entirely in the codebase). - We never used the Jetpack functionality, and we were leaking a lot of context objects that could theoretically result in memory leaks. - The migration of view models in this way has already been ongoing in the codebase; this PR just finishes moving the rest of them over to remove the deprecated JetPack view model reference. - Note that this doesn't actually change the scope of the view models, and in fact they should largely behave as they always have. - ``ObservableViewModel`` was subsequently updated, and may be something we could remove in the future now that it's no longer a Jetpack view model. - The old view model binding code was removed, along with its test file exemptions. It's no longer used now that the view models have been finished being migrated over to direct injection. - Some of the binding adapters didn't correctly correspond to their namespaced properties. I _think_ that the databinding compiler was still hooking them up correctly, but they produced build warnings that have now been addressed (specifically, 'app' is implied). Some other properties were using unusual namespaces, so these were replaced with 'app' versions for consistency & correctness. - Some cases where SAM interfaces could be converted to lambdas were also addressed (mainly for ``Observer`` callbacks in UI code). - ``DrawerLayout.setDrawerListener`` was replaced with calls to ``DrawerLayout.addDrawerListener`` since the former [is deprecated](https://developer.android.com/reference/androidx/drawerlayout/widget/DrawerLayout#setDrawerListener(androidx.drawerlayout.widget.DrawerLayout.DrawerListener)). This isn't expected to have a functional difference. - Some other minor control flow warnings were addressed (such as dead code paths). - ``when`` cases were updated to be comprehensive (as this is enforced starting in newer versions of Kotlin even for non-result based ``when`` statements). - Some unused variables were removed and/or replaced with ``_`` per Kotlin convention. - Some parameter names needed to be updated to match their override signatures. - One change in ``ExitSurveyConfirmationDialogFragment`` involved removing parsing a profile ID. Technically this is a semantic change since now a crash isn't going to happen if the profile ID is missing or incorrect, but that seems fine since the fragment doesn't even need a profile ID to be passed. - Some of the test activities were updated to bind a ``Runnable`` callback rather than binding to a method (just to avoid passing the unused ``View`` parameter and to keep things a bit simple binding-wise). - Some cases were fixed where variables were being shadowed due to reused names in deeper scopes. - There were some typing issues going on in tests with custom test application components. This has been fixed by explicitly declaring the application component types rather than them being implicit within the generated Dagger code. - ``getDrawable`` calls were updated to pass in a ``Theme`` since the non-theme version is deprecated. - Some Java property references were updated, too (i.e. using property syntax instead of Java getters when referencing Java code in Kotlin). - In some cases, deprecated APIs were suppressed since they're needed for testing purposes. - Mockito's ``verifyZeroInteractions`` has been deprecated in favor of ``verifyNoMoreInteractions``, so updates were made in tests accordingly. - ``ExperimentalCoroutinesApi`` and ``InternalCoroutinesApi`` have been deprecated in favor of a newer ``OptIn`` method (which can actually be done via kotlinc arguments, but not in this PR). Thus, they've been outright removed in cases where not needed, and otherwise migrated to the ``OptIn`` approach where they do need to be declared. - In some cases, Kotlin recommends using a ``toSet()`` conversion for iterable operations when it's more performant, so some of those cases (where noticed) have been addressed. - Some unused parameter cases needed to be suppressed for situations when Robolectric is using reflection to access them. - In some cases Android Studio would recommend transformation chain simplifications; these were adopted where obvious. - There are a few new TODOs added on #3616 as well, to clean up deprecated references that have been suppressed in this PR. - ``BundleExtensions`` was updated to implement its own version of the type-based ``getSerializable`` until such time as ``BundleCompat`` can be used, instead (per #5405). - A **lot** of nullability improvements needed to happen throughout the JSON asset loading process since there was a lot of loose typing happening there. - Some Kotlin & OkHttp deprecated API references were also updated to use their non-deprecated replacements. - ``NetworkLoggingInterceptorTest`` was majorly reworked to ensure that the assertions would actually run (``runBlockingTest`` was being used which is deprecated, and something I try to avoid since it's very difficult to write tests that use it correctly). My investigations showed that the assertions weren't being called, so these tests would never fail. The new versions will always run the assertions or fail before reaching them, and fortunately the code under test passes the assertions correctly. Ditto for ``ConsoleLoggerTest``. - Some parts of ``SurveyProgressController`` were reworked to have better typing management and to reduce the need for nullability management. - Some generic typing trickiness needed to be fixed ahead of the Kotlin version upgrade in ``UrlImageParser``. See file comments & links in those comments for more context. - ``BundleExtensionsTest`` had to be changed since ``getSerializableExtra`` is now deprecated. We also can't update the test to run SDK 33 since that requires upgrading Robolectric, and Robolectric can't be upgraded without upgrading other dependencies that eventually lead to needing to upgrade both Kotlin and Bazel (so it's a non-starter; this is a workaround until we can actually move to a newer version of Robolectric). - There was some minor code-deduplication & cleanup done in ``ClickableAreasImage``. - Some incorrect comments were removed in tests (to the effect of "using not-allowed-listed variables should result in a failure."). These seemed to have been copied from an earlier test, but the later tests weren't actually verifying that behavior so the comment wasn't correct. - An unused method was removed from ``ConceptCardRetriever`` (``createWrittenTranslationFromJson``) and some other small cleanup/consolidation work happened in that class. - Some stylistic changes were done in ``TopicController`` for JSON loading to better manage nullable situations, and to try and make the JSON loading code slightly more Kotlin idiomatic. Note that overall the PR has relied **heavily** on tooling to detect warnings to fix, and automated tests to verify that the changes have no side effects. Note also that this PR does not actually enable warnings-as-errors; that will happen in a downstream PR. ## Essential Checklist - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only N/A -- While this changes UI code, it should change very few UI behaviors and only failure cases for those it does affect. It's largely infrastructural-only and falls mainly under refactoring/cleanup work. --------- Co-authored-by: Adhiambo Peres <[email protected]> Co-authored-by: Sean Lip <[email protected]>
|
@oyeraghib just confirming that you are working on this, and that you are not blocked? |
|
Hi @adhiamboperes I have been having the issue with making Oppia Android build on my Mac. I never faced this issue with Windows before. Errors while building : Also I was having another issue with protobuf which I fixed by doing to this |
|
@oyeraghib, it looks like you have run into multiple set up issues. Could you please copy your comment to the discussion board at this link: https://github.com/oppia/oppia-android/discussions/categories/q-a-installation, so that we can discuss them in detail? |
|
Sure done.! |
|
@oyeraghib are you still working on this issue ? @adhiamboperes can i work on this issue ? |
6 tasks
adhiamboperes
added a commit
that referenced
this issue
Jul 16, 2024
<!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation Fixes #5406 Replaced deprecated InputMethodManager.SHOW_FORCED with '0' flag to ensure compatibility with SDK 33. Updated the following files: StateFragmentPresenter.kt SurveyFragmentPresenter.kt QuestionPlayerFragmentPresenter.kt These changes ensure that the keyboard hides as expected across the app scenarios, maintaining the intended behavior. ## Essential Checklist <!-- Please tick the relevant boxes by putting an "x" in them. --> - [x] The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".) - [ ] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). --------- Co-authored-by: Mr. 17 <[email protected]> Co-authored-by: Adhiambo Peres <[email protected]>
|
@adhiamboperes, with the fix (#5463), can we remove the TODOs in the following files since they cause script check failures?
Thanks! |
|
@Rd4dev, the Todos were removed(unless I missed something) have you synced with develop? |
|
Ah, apologies @adhiamboperes, I haven't synced the chains yet. Thanks for the update. I'll check after syncing and confirm if persists. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
SDK 33 deprecated
InputMethodManager.SHOW_FORCED(see https://developer.android.com/reference/android/view/inputmethod/InputMethodManager#SHOW_FORCED) which is used to hide the keyboard via a call toInputMethodManager.hideSoftInputFromWindow.#5402 helped reveal which activities need updating:
Describe the solution you'd like
It actually seems like the original implementation is incorrect.
hideSoftInputFromWindowexpects flag values of0or a combination ofHIDE_IMPLICIT_ONLYandHIDE_NOT_ALWAYS.Unfortunately, the documentation is unclear on what '0' indicates, but the other flags seem to indicate cases when the keyboard shouldn't be hidden so it's my interpretation that '0' means 'force hide when possible' (which is what we seem to want in this situation). Thus, the fix seems to be to migrate over to using the '0' constant.
Describe alternatives you've considered
No response
Additional context
Note that fixes to the files listed above need to be carefully tested to ensure that the keyboard does still hide when expected for each of the scenarios we expect it to hide.
The text was updated successfully, but these errors were encountered: