Skip to content
This repository has been archived by the owner on Sep 3, 2023. It is now read-only.

Switch to ListView + ListAdapter for dialog lists #72

Merged
merged 17 commits into from Dec 2, 2014

Conversation

ZacSweers
Copy link
Contributor

This pull request is somewhat big, and switches over the list implementation to a ListView + ListAdapter style.

This accomplishes two important things:

  • It brings the API to be more in line with a real dialog (think setAdapter() and getListView())
  • Dramatically improves list scrolling performance due to not inflating all views upfront.

The basic structure of this is that it uses a new in-house adapter called MaterialDialogAdapter for first-party list options (single, multi, and regular). For custom adapters (set via .adapter() in the Builder), it delegates their logic to the user's implementation (like a real dialog!).

This also improved performance of handling callbacks, as now we can retrieve the item directly rather than iterate over the inflated views to find the selected one.

Here's a short screen record going through the list items in the sample: https://www.dropbox.com/s/gs8qn6ytrktrc6d/device-2014-12-01-182852.mp4?dl=0

NOTE: This removes the ItemProcessor API, as it's now unnecessary since users can build the dialog with their own (much more flexible) adapter.

I made a couple other minor changes when I saw them, like c2ad109

@ZacSweers
Copy link
Contributor Author

My coworker @emilsjolander also looked over the code before I pushed it, so should be pretty bug free

@afollestad
Copy link
Owner

Is there a reason you chose not to use RecyclerView? It would be smoother and simpler than a ListView, but it looks good

@ZacSweers
Copy link
Contributor Author

RecyclerView adds an extra transient dependency that developers may not want as part of the library. It also wouldn't really make a difference in this case.

Per the developer docs:

Use the RecyclerView widget when you have data collections whose elements change at runtime based on user action or network events.

Since the first party adapters are just simple and not updating items dynamically, there's no need for it. They're simple enough views as well that there's no noticeable performance difference.

afollestad added a commit that referenced this pull request Dec 2, 2014
Switch to ListView + ListAdapter for dialog lists
@afollestad afollestad merged commit a77928f into afollestad:master Dec 2, 2014
@afollestad
Copy link
Owner

I've noticed wrong padding used in a few places but I'll have it fixed.

Otherwise it looks great, thanks

@ZacSweers
Copy link
Contributor Author

I was going by the material spec for dialogs, mainly because I noticed some places had extra side padding beneath the title. Is that what you're referring to?

@afollestad
Copy link
Owner

Well the one that I just noticed was below the list, there should be 16dp between the content and action buttons, but there was 24dp.

I'm fixing the space above the list view when there's no title, too.

@ZacSweers
Copy link
Contributor Author

Ah, alrighty

@afollestad
Copy link
Owner

There was one logic error too :) 0.3.2 should now be available on Gradle, it may take a minute for Android Studio to see that it's live

@afollestad
Copy link
Owner

Turns out there was actually an issue created from the latest code, for whatever reason Gradle doesn't want to generate Javadoc while uploading to Bintray which prevents the entire build cycle from finishing. I'll have to look into it.

@ZacSweers
Copy link
Contributor Author

Weird. Keep me posted!
On Mon, Dec 1, 2014 at 7:58 PM Aidan Follestad notifications@github.com
wrote:

Turns out there was actually an issue created from the latest code, for
whatever reason Gradle doesn't want to generate Javadoc while uploading to
Bintray which prevents the entire build cycle from finishing. I'll have to
look into it.


Reply to this email directly or view it on GitHub
#72 (comment)
.

@ZacSweers
Copy link
Contributor Author

There was a place where I put @returns instead of @return, though I don't think I've seen any issues with that before

@afollestad
Copy link
Owner

I'll see if that's it. Out of nowhere, the sample app starting crashing even though nothing was changed, so there's yet another thing to fix before I can publish it :o

@afollestad
Copy link
Owner

Got that fixed, one last thing before it can be published: the MaterialDialog class wasn't updated to display the divider for the ListView, since canContentScroll() wasn't updated to detect it.

The update will be up very soon.

@ZacSweers
Copy link
Contributor Author

Are dividers shown for dialogs? Looking at the material spec, I only see one possible example of it: http://www.google.com/design/spec/components/dialogs.html#dialogs-confirmation-dialogs

@afollestad
Copy link
Owner

Yeah, if the content is scrollable a divider is supposed to be displayed. The wifi connection dialog and ringtone selection examples on that page are examples.

@ZacSweers
Copy link
Contributor Author

Gotcha

@afollestad
Copy link
Owner

Alright, 0.3.2 should finally be available via the Gradle dependency

@ZacSweers
Copy link
Contributor Author

awesome 👍

@fstephany
Copy link

This is great, thanks guys!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants