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.

Sign up for GitHub

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

Jump to bottom

Add ability to delete reference layers #6175

Merged
merged 17 commits into from
Jun 13, 2024
Merged

Add ability to delete reference layers #6175

merged 17 commits into from
Jun 13, 2024

Conversation

grzesiek2010
Copy link
Member

@grzesiek2010 grzesiek2010 commented Jun 7, 2024 •
edited
Loading

Closes #5850
Closes #6183

Why is this the best possible solution? Were any other approaches considered?

The issue does not describe how the confirmation dialog (when deleting layers) should look like but the one I saw on Figma was pretty complex. I'm not sure if we want/should have the complex one so for now I've implemented something simple. If we want to change it we can do that in a separate pull request:
image Screenshot_1718056552

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Once this pr is merged it's time to test everything (adding/removing/setting offline layers). Please read the issues marked as mbtiles import and verify the new functionality.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@grzesiek2010 grzesiek2010 marked this pull request as ready for review June 10, 2024 22:00
seadowg
seadowg requested changes Jun 11, 2024
View reviewed changes
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6175 (comment)

seadowg
seadowg requested changes Jun 11, 2024
View reviewed changes
Copy link
Member

@seadowg seadowg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#6175 (comment)

@grzesiek2010 grzesiek2010 force-pushed the COLLECT-5850 branch 3 times, most recently from f345b6a to 620f902 Compare June 11, 2024 18:47
seadowg
seadowg requested changes Jun 12, 2024
View reviewed changes
maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersPicker.kt Outdated Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersPicker.kt Outdated Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersPicker.kt Outdated Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersPicker.kt Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersPickerAdapter.kt
return oldItem == newItem
}
}
private val asyncListDiffer = AsyncListDiffer(this, diffUtil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen AsyncListDiffer before. Is this just a performance optimization, or does it not working with it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would work without it. I've added it for performance optimization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks from the docs like we could just be using ListAdapter instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the sample below in https://developer.android.com/reference/androidx/recyclerview/widget/ListAdapter shows that it doesn't work out of the box. There is a DiffUtil.ItemCallbac similar to what I've implemented.

maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersStateViewModel.kt Outdated Show resolved Hide resolved
maps/src/main/java/org/odk/collect/maps/layers/OfflineMapLayersStateViewModel.kt Outdated
import org.odk.collect.androidshared.livedata.MutableNonNullLiveData
import org.odk.collect.androidshared.livedata.NonNullLiveData

class OfflineMapLayersStateViewModel(checkedLayerId: String?) : ViewModel() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to do have anything to do with layers now right? It could be named something generic and moved to lists. I'm also wondering if we could reduce this to just the "checked"/select state and use MultiSelectViewModel to track the expanded state. I think we'd just need to modify the latter to use String IDs instead of Long (which is probably better anyway).

What do you think? Better as a follow up PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better as a follow up PR?

Yes for now I've moved it to #6174 (comment)

maps/src/main/res/layout/offline_map_layers_picker_item.xml Outdated Show resolved Hide resolved
maps/src/test/java/org/odk/collect/maps/layers/OfflineMapLayersPickerTest.kt Outdated Show resolved Hide resolved
maps/src/test/java/org/odk/collect/maps/layers/OfflineMapLayersPickerTest.kt

scheduler.flush()

onView(withRecyclerView(R.id.layers).atPositionOnView(1, R.id.arrow)).perform(click())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these RecyclerView Espresso assertions and actions make the tests pretty hard to read, and you'll see there are quite a lot of changes if you auto format the file due to the line lengths. Maybe you can pull out some helpers to try and make them a little cleaner. Remember that the result of an onView call can be pulled out to a local variable which can save a bunch of repetition and line length. The chained called can also be stacked on new lines like:

onView(...
     .atPositionOnView(...
     .check(...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that and I'm on it now... if it's the last missing thing please merge and I will add it along with other improvements #6174 (comment)

@seadowg
Copy link
Member

seadowg commented Jun 12, 2024

@grzesiek2010 really liking where this has ended up! Just a few more comments with some questions and suggested tweaks.

@grzesiek2010 grzesiek2010 mentioned this pull request Jun 12, 2024
Open
11 tasks
@seadowg
Copy link
Member

seadowg commented Jun 13, 2024

This looks great! I've left a couple of comments about some small tweaks, but nothing that really blocks this if you just want to follow up with them.

@grzesiek2010 grzesiek2010 merged commit 515a8f9 into getodk:master Jun 13, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seadowg seadowg seadowg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Offline layer set in settings not applied in Select one from map form Add ability to delete reference layers
2 participants
@grzesiek2010 @seadowg