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

Facelet adapter not properly synchronized with backing list in InterfacesFragment #31

Closed
jordanauge opened this issue Jul 21, 2020 · 0 comments

Comments

@jordanauge
Copy link
Contributor

When the hICN stack stops (eg. because of inactivity), and the user is on the Interfaces tab displaying the list of faces, the application crashes.

This is due to a clear() on the backing list without notifying about a change in dataset in InterfacesFragment.java. Some propose to notify just after the change in dataset, but the alternative (and preferred here) solution is to directly call clear() on the adapter itself, in particular for uniformity with the rest of the code (add()).

I expect the call through the adapter would solve potential race conditions between the list update and the time when the adapter is notified, as the method could take care of running everything in the UI thread. However the current code is explicitely calling those methods in the UI thread, so for safety and until confirmed, the clear() has also been moved in the UI thread. This choice is further motivated by the last change.

By default, setNotifyOnChange is true, which means the adapter takes care of calling notifyDatasetChanged after any modification. The code was thus unnecessarily calling it after adding elements, since this was redundant, but likely inserted for optimization purposes. The setNotifyOnChange has thus been set to false, and the notification is explicitely in both clear() and add(). With such delayed operations, i doubt the code could ever be safe otherwise.

Additional note: this code never clears the list of facelets unless the face manager is found in a stopped state: this is okay as soon as facelets are never removed in the face manager, which is indeed the case. A comment has been added in the file to clarify this aspect.

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

1 participant