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

getPosition(SOME_ITEM_ID) returning -1 when account switcher menu is shown #2826

Open
nikclayton opened this issue Feb 3, 2025 · 4 comments

Comments

@nikclayton
Copy link
Contributor

nikclayton commented Feb 3, 2025

Edit: This is with version 9.0.2.

This sequence of events:

  1. Start the app.
  2. Open the drawer, to which an AccountHeaderView has been attached.
  3. Tap the account switcher arrow to show the list of account items.
  4. Leave the app (e.g., press Home button).
  5. Relaunch the app.

At this point I think the drawer reloads the saved state, which seems to include the state that the account menu is being shown instead of the regular menu.

This means calls like slider.getPosition(SOME_ITEM_ID) return -1 instead of the actual position of SOME_ITEM_ID in the main drawer.

Interestingly, in the same scenario slider.itemAdapter.getAdapterPosition(SOME_ITEM_ID) does return a positive position (although it appears to be one less than the actual position).

Not sure if this is a bug, and slider.getPosition() should not return -1 in this case, or if this is expected behaviour and there's some mechanism to "wait until the account switcher menu is closed, then update the main drawer items" on resume that I'm missing.

@nikclayton
Copy link
Contributor Author

nikclayton commented Feb 4, 2025

Poking at the code some more it looks like there's adapter (which wraps all the individual adapters), and then different individual adapters for the header contents, the primary drawer contents, secondary drawer contents (if the account switcher arrow has been tapped), and so on.

This is getPosition:

fun MaterialDrawerSliderView.getPosition(identifier: Long): Int {
    return this.getPositionByIdentifier(identifier)
}

and this is getPositionByIdentifier:

@Deprecated("Please use getPosition instead", ReplaceWith("getPosition"))
fun MaterialDrawerSliderView.getPositionByIdentifier(identifier: Long): Int {
    if (identifier != -1L) {
        for (i in 0 until adapter.itemCount) {
            if (adapter.getItem(i)?.identifier == identifier) {
                return i
            }
        }
    }

    return -1
}

The call to adapter.itemCount looks suspicious. itemCount is documented as "Calculates the total ItemCount over all registered adapters" and "Returns: the global count".

In practice, it appears to count the displayed items. I checked this by adding this code to an activity that contains a drawer:

lifecycleScope.launch {
    while(true) {
        delay(5000)
        Timber.w("Drawer item count: ${binding.mainDrawer.adapter.itemCount}")
    }
}

then checking the logs while toggling the account switcher arrow.

Showing the contents of the primary item adapter this logged the item count as 28. After clicking the account switcher toggle this logged the item count as 3.

28 is consistent with the number of items in the primary adapter, and 3 is consistent with the number of items in the secondary (account) adapter (after adding 1 to account for the item in the header adapter).

nikclayton added a commit to nikclayton/pachli-android that referenced this issue Feb 4, 2025
Previous code would crash if the app resumed when the account selector
was open, as `getPosition` would not return the position of a primary
item (see mikepenz/MaterialDrawer#2826 for
details).

Fix this by directly checking the primary `itemAdapter` instead of
using `getPosition`.
nikclayton added a commit to pachli/pachli-android that referenced this issue Feb 4, 2025
Previous code would crash if the app resumed when the account selector
was open, as `getPosition` would not return the position of a primary
item (see mikepenz/MaterialDrawer#2826 for
details).

Fix this by directly checking the primary `itemAdapter` instead of using
`getPosition`.
@mikepenz
Copy link
Owner

mikepenz commented Feb 6, 2025

Good day @nikclayton

The MaterialDrawer uses the FastAdapter library under the hood which powers the adapter containing the drawer items.

As you already highlighted the MaterialDrawer will switch out adapters when the expanded drawer is displayed and thus positions themself change including the main adapter.

When the state is restored it will be applied respectively: https://github.com/mikepenz/MaterialDrawer/blob/develop/materialdrawer/src/main/java/com/mikepenz/materialdrawer/widget/MaterialDrawerSliderView.kt#L512-L523

@nikclayton
Copy link
Contributor Author

Right. But the problem is that, when the app relaunches and the state is restored, I need to modify the primary menu items.

This is because the primary menu contains items based on data from the user's server. The user may have updated these items on the server (eg., via a web app) and it's important those changes are reflected in the menu when the user resumes.

I can update just the primary menu using itemAdapter directly (that's what I did in pachli/pachli-android#1258) but if there's a better way (like, a mechanism for listening until the menu is about to be displayed, analogous to onPrepareOptionsMenu) I'll use that. I couldn't find an API for that, but maybe I missed something.

@mikepenz
Copy link
Owner

mikepenz commented Feb 6, 2025

I understand.

Updating the items via the itemAdapter is certainly not that wrong, given the extension functions to update also fully depend on the same mechanisms. And you likely can stay within the scope of the singular itemAdapter for your usecase.

Practically having extension functions that solely base on the itemAdapter and do not take the full adapter into account will likely not be too bad to establish.


You can retrieve the info if the drawer is currently switched switchedDrawerContent, but your approach is nicer as it wouldn't be prone to race conditions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants