-
Notifications
You must be signed in to change notification settings - Fork 315
Patients List UI screen & button for old screen launch. #72
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
Conversation
pankajkatoch
commented
Jun 29, 2020
- Added a sample patient list Recyclable View that shows 20 patients with gender and date of birth
- Uses PatientListViewModel to monitor live data, and update the view holders
- Added a floating button to launch the old Main activity that loads Fhir and CQL resources.
- Loads Fhir bundle with 20 Patients from a Json file in assets folder.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Pankaj!
android:name=".SamplePatientListActivity" | ||
android:label="@string/title_samplepatient_list" | ||
android:theme="@style/AppTheme.NoActionBar"> | ||
<intent-filter> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this clash with the same intent filter for .MainActivity below? I would try to reduce duplication by removing the same intent filter in the other one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the action.MAIN and category.LAUNCHER from the erstwhile MainActivity (will be called CqlEvaluationActivity), for now.
Please note that it's fine for multiple activities to be marked as MAIN/LAUNCHER, as it's supposed to create different icons by the Home Launcher App. For the demo purpose, we may subsequently add loading of CQL library and Fhir resources as a separate Main Activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename the main activity to cql evaluation?
@@ -0,0 +1,37 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this design - but I just don't think we should focus on this right now because it will create additional maintenance cost. For simplicity - can we just stick to the two screen design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -0,0 +1,41 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this file: activity_patient_list.xml
I don't think the word sample adds much. Also making sure each word id separated by '_'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing "sample" from list, and details related components.
<include layout="@layout/samplepatient_list" /> | ||
</FrameLayout> | ||
|
||
<com.google.android.material.floatingactionbutton.FloatingActionButton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would hide/remove this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiding it.
android:layout_width="wrap_content" android:layout_height="wrap_content"> | ||
|
||
<TextView | ||
android:id="@+id/id_text" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please change this to a more descriptive name than id_text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
example/src/main/AndroidManifest.xml
Outdated
android:name="android.support.PARENT_ACTIVITY" | ||
android:value="com.google.fhirengine.example.SamplePatientListActivity" /> | ||
</activity> | ||
<activity android:name=".MainActivity"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename this activity if it's not going to be the main activity any more? can you change it to CqlEvaluationActivity or something similar? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Will do it in the upcoming Details UI PR, as other folks seem to be adding many init features in the MainActivity.
val fhirContext = FhirContext.forR4() | ||
fhirBundle = fhirContext.newJsonParser().parseResource(Bundle::class.java, | ||
jsonString) as Bundle | ||
for (i in 1..fhirBundle.entry.size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use forEach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
object SamplePatients { | ||
private val PATIENTS: MutableList<PatientItem> = ArrayList() | ||
private val PATIENTS_MAP: MutableMap<String, PatientItem> = HashMap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add comment to describe this map (from what to what)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to lowercase.
use mutableMapOf()
for initialisation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done & done.
* Parses and loads Patient data from the passed JSON String into items that could be used by | ||
* PatientListViewModel. | ||
*/ | ||
object SamplePatients { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a helper class, I would rather not declare it as an object that holds the actual data. I would just make this a utility class without state. It doesn't look like you need the internal state anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point!
Done.
// Trigger value update in the view holder when underlying viewmodel changes. | ||
viewModel.getPatients().observe( | ||
parentActivity, Observer<List<SamplePatients.PatientItem>> { patients -> | ||
patientValues = patients |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use "it" rather than declaring patients as the variable name in this lambda. If you do that you can also rename the patientValues field in this class to patients (which is shorter).
So this lambda becomes:
{ patients = it }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
example/build.gradle
Outdated
implementation "com.squareup.retrofit2:converter-gson:$retrofitVersion" | ||
implementation "com.squareup.retrofit2:retrofit-mock:$retrofitVersion" | ||
implementation "com.squareup.okhttp3:logging-interceptor:$okhttpLoggingInterceptorVersion" | ||
implementation 'androidx.legacy:legacy-support-v4:1.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. it was added by Android Studio at some point.
Removed.
example/build.gradle
Outdated
implementation "com.squareup.retrofit2:retrofit-mock:$retrofitVersion" | ||
implementation "com.squareup.okhttp3:logging-interceptor:$okhttpLoggingInterceptorVersion" | ||
implementation 'androidx.legacy:legacy-support-v4:1.0.0' | ||
implementation 'androidx.recyclerview:recyclerview:1.1.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create constants for the versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* possibly patient details, on a two pane device. | ||
*/ | ||
class SampleItemRecyclerViewAdapter( | ||
private val parentActivity: SamplePatientListActivity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't pass the activity and the view model to the adapter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was an artefact of Android Studio generated Master-Details template, and there it creates this RecylerViewAdapter as an inner class within the List Activity. See Example: FooItemListActivity.kt
Changed it now to use the ListAdapter.
private lateinit var patientValues: List<SamplePatients.PatientItem> | ||
|
||
init { | ||
onClickListener = View.OnClickListener { v -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all of this logic should be handled by the activity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
// Trigger value update in the view holder when underlying viewmodel changes. | ||
viewModel.getPatients().observe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the activity should observe the patients and pass them on to the adapter via ListAdapter#submitList
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
object SamplePatients { | ||
private val PATIENTS: MutableList<PatientItem> = ArrayList() | ||
private val PATIENTS_MAP: MutableMap<String, PatientItem> = HashMap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to lowercase.
use mutableMapOf()
for initialisation
private val PATIENTS_MAP: MutableMap<String, PatientItem> = HashMap() | ||
|
||
// The resource bundle with Patient objects. | ||
private lateinit var fhirBundle: Bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep this as nullable, rather than lateinit
as you don't know when getPatientItems
is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I thought that since getPatientItems
was the only public function, so it should've been safe.
*/ | ||
fun getPatientItems(jsonString: String): MutableList<PatientItem> { | ||
val fhirContext = FhirContext.forR4() | ||
fhirBundle = fhirContext.newJsonParser().parseResource(Bundle::class.java, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this creates a parser every time. can we just get the parser as a parameter when we get the jsonString?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used companion object for creating the parser once. Done.
- Refactored SampleItemRecyclerViewAdapter to use ListAdapter. - Clean separation of Acitivity, ViewHolder and Adapter. - Improved usage of Kotlin idioms. - Removed handling of two panes for a tablet device. - Removed remenanats of boilerplate code from Master-Details template generated code.
Handled all the review comments and suggestions. A couple of them would be handled in the Details PR shortly.
|
} | ||
} | ||
|
||
class PatientItemViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be internal
or private
. Default (i.e. omitted) is public
. See: https://kotlinlang.org/docs/tutorials/kotlin-for-py/visibility-modifiers.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needed to be in its own file. Moved it.
@@ -1,37 +1,36 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the license got removed. For non-java/kotlin files, you have to add it yourself: https://github.com/google/android-fhir#license-headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer.
Ran addlicense for all the missing license headers in xml files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
* The ViewModel helper class for SampleItemRecyclerViewAdapter, that is responsible for preparing | ||
* data for UI. | ||
*/ | ||
class PatientListViewModel(jsonString: String) : ViewModel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, patients are downloaded when calling FhirEngine.sync
. Do we need the json parsing if we can just get them from the FhirEngine
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will switch to actual patients data in real_data PR next.
Will still likely keep the json parsing facility around, in case
- DB is broken or sync isn't working
- For showcasing some Fhir objects that are not readily available to sync.
* possibly patient details, on a two pane device. | ||
*/ | ||
class SampleItemRecyclerViewAdapter( | ||
private val itemOnClickListener: View.OnClickListener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of passing a View.OnClickListener
, pass a function here:
private val onItemClicked: () -> Unit
then in the ViewHolder, just call the function in the OnClickListener
implementation.
The caller outside this adapter, shouldn't know (or care) if we're implementing an OnClickListener or whatever other type. It should just care what is the action that should be triggered when an item is clicked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Moved setting the of onClickListener to PatientItemViewHolder.
setSupportActionBar(findViewById(R.id.detail_toolbar)) | ||
|
||
findViewById<FloatingActionButton>(R.id.fab).setOnClickListener { view -> | ||
Snackbar.make(view, "Replace with your own detail action", Snackbar.LENGTH_LONG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we already remove it now?
|
||
/** | ||
* A fragment representing a single SamplePatient detail screen. | ||
* This fragment is either contained in a [SamplePatientListActivity] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this removed? The comment still covers 2 panes
/** | ||
* The dummy content this fragment is presenting. | ||
*/ | ||
private var item: DummyDetailsContent.DummyItem? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get rid of all the DummyDetailsContent
and use the actual data we're interested in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the 2 panes and floating button related code/comments.
Keeping DummyDetailsContent in the interest of keeping the app fully functional for each PR.
Will be remove in the Details PR.
* | ||
* TODO: Replace all uses of this class before publishing your app. | ||
*/ | ||
object DummyDetailsContent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's delete this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to comment above. Will be removed in Details PR.
…lder. Also removed reference to 2 panes. Ran Spotless.
android:supportsRtl="true" | ||
android:theme="@style/AppTheme"> | ||
<activity | ||
android:name=".SamplePatientListActivity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we decided we'll use real data, we shouldn't name these things as sample. Whether it is sample or not has nothing to do with this activity anyway, it's to do with the data layer. So let's remove the word sample from this activity class name, as well as the PatientDetailsActivity class name, and also the patient details xml file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All instances of word sample have already been removed in upcoming Details UI PR, which I'm waiting to send for review, as I soon as I merge this one to the master.
That also will take care of renaming the MainActivity
to CqlEvaluationActivity
.
android:name=".SamplePatientListActivity" | ||
android:label="@string/title_samplepatient_list" | ||
android:theme="@style/AppTheme.NoActionBar"> | ||
<intent-filter> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename the main activity to cql evaluation?
override fun areItemsTheSame( | ||
oldItem: SamplePatients.PatientItem, | ||
newItem: SamplePatients.PatientItem | ||
): Boolean = oldItem == newItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should just compare the id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done for the comparison.
import androidx.appcompat.app.AppCompatActivity | ||
|
||
/** | ||
* An activity representing a single SamplePatient detail screen. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit :P the comments can fit in fewer lines (line wrapping too aggressive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Installed Wrap to Column Plugin, and then Ctrl-Shift-Cmd-W
on Macs.
override fun onOptionsItemSelected(item: MenuItem) = | ||
when (item.itemId) { | ||
android.R.id.home -> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: additional new lines can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small things.
lgtm after they're addressed
R.id.dob | ||
) | ||
|
||
fun bindTo(patientItem: SamplePatients.PatientItem, onItemClicked: (View) -> Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if I'm not mistaken, you don't need the view here. maybe you need the PatientItem
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
val recyclerView: RecyclerView = findViewById(R.id.samplepatient_list) | ||
|
||
// Click handler to help display the details about the patients from the list. | ||
val onPatientItemClicked: (View) -> Unit = { v -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the previous comment, you don't need the view here, you need the PatientItem
. You can just pass it instead of the view. Like this you don't need to use the tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
val intent = Intent(v.context, SamplePatientDetailActivity::class.java).apply { | ||
putExtra(SamplePatientDetailFragment.ARG_ITEM_ID, item.id) | ||
} | ||
v.context.startActivity(intent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're already in an activity. You should be able to do something like this.startActivity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Used this.startActivity and this.applicationContext
val adapter = SampleItemRecyclerViewAdapter(onPatientItemClicked) | ||
recyclerView.adapter = adapter | ||
|
||
patientListViewModel.getPatients().observe(this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use observe
from core-ktx to simplify this.
import androidx.lifecycle.observe
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The observe()
here in patientListViewModel.getPatients().observe()
is an instance of androidx.lifecycle.observe
.
Here I'm observing any changes to LiveData
returned from patientListViewModel.getPatients()
- and if so then submitting the list to adapter to update the view.
Are there other better ways of using it?
private fun getJsonStrForPatientData(): String { | ||
val patientJsonFilename = "sample_patients_bundle.json" | ||
|
||
return this.applicationContext.assets.open(patientJsonFilename).bufferedReader().use { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're in an activity, so you should be able to just do assets.open...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
class SamplePatients { | ||
private val patients: MutableList<PatientItem> = ArrayList() | ||
// Maps a patient position to PatientItem, used to display both the position and details. | ||
private val patients_map: MutableMap<String, PatientItem> = mutableMapOf() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to something more descriptive. Maybe idsPatients
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
class SamplePatients { | ||
private val patients: MutableList<PatientItem> = ArrayList() | ||
// Maps a patient position to PatientItem, used to display both the position and details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the position a string?
looking lower in the code, it looks like this is not a position, but a patient id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
Maps a temporary patient id to PatientItem, used to display patient information both in list and details.
|
||
private fun addPatientItem(item: PatientItem) { | ||
patients.add(item) | ||
patients_map[item.id] = item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the map is just the patients, associated by item id
you can construct the map automatically from patients by doing
patients.associateBy { it.id }
after you have constructed the list of patients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Using the kotlin idiom. See the comment below.
fun getPatientItems(jsonString: String): List<PatientItem> { | ||
fhirBundle = fhirJsonParser.parseResource(Bundle::class.java, | ||
jsonString) as Bundle | ||
(1..fhirBundle!!.entry.size).forEach { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this can be written more idiomatic like this:
patients.addAll(fhirBundle?.entry)
patients_map.addAll(patients.associateBy { patient -> patient.id} )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. this is what makes kotlin so nice to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean patients_map.putAll()
?
Since id is a temporary index for display purpose, fhirBundle?.entry?.resource
can't be used directly in the PatientItemList
. However you're right no need to use forEach here.
Used a combination of mapIndexed
and let to call createPatientItem
for each fhirPatient resource.
Also use take()
to simply limit patient list size to MAX_RESOURCE_COUNT
class SamplePatients { | ||
private val patients: MutableList<PatientItem> = ArrayList() | ||
// Maps a patient position to PatientItem, used to display both the position and details. | ||
private val patients_map: MutableMap<String, PatientItem> = mutableMapOf() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like we're using this anywhere. do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Used in the Details Page/UI.
* Used kotlin idioms while creating list and maps of PatientItems. * Removed unnecessary passing of view argument in itemClicked handler. * Ran spotless check and apply. * Removal of all remaining occurances of term "sample" would be Details UI PR.