Open Gaket opened 6 years ago
MaterialAboutToggleItem? Don't think I've created one of these - is it in a forked library. I've actually wanted to implement some items like this so that people can use the library as a settings page also.
Also, it should definitely be possible to implement.
Hi, this functionality has been implemented in a fork at by @filol here: https://github.com/filol/material-preference-library
@daniel-stoneuk Perhaps you can close this issue as it was merged?
@Robyer we haven't merged it into the main branch yet since it's performance is pretty terrible.
@daniel-stoneuk Aha, I saw on the MaterialPreferenceLibrary notice that that library is deprecated now as it was merged to MAL. And can you be more specific? What kind of performance issues are there?
Also, I'm comparing the changes between your branch and master and I noticed some weird things:
why is there new INTERNET permission in AndroidManifest? // EDIT: I see, it's only in demo app and because of licenseadapter
why is there areContentsTheSame
method that calls getDetailString() to create big string representations of the objects and then compares the two strings? Why not just properly implement equals() method in first place? // EDIT: I understand now, that it should compare only visual changed, not all of them, that's why equals is not wanted. But still, it shouldn't compare created strings, but maybe rather the values directly. But that's small issue.
if the performance issues you are talking about are when you set checked listener and then deny the checked state as in that example, it is caused by this: we are setting this.aCheckBox.setOnCheckedChangeListener(
, but this is called always, when the internal checked state changed, i.e. even when it is programatically changed by calling "setChecked". So in example here we are inside checked change listener, and here we call setChecked(true)
again, which will as a result call our checked change listener again, where we will again call it... Also notice that we return false from this method (to notify that we don't want to allow the change). So we may just remove that manual call to switchItem.setChecked(true);
and theoretically it should work. BUT it is implemented here with the same infinite recursion problem - if user returns false, then we call the setChecked(!isChecked)
which in turn again calls our own listener, where we call that again... (second infinite recursion). Reason why it won't end up in app crash is that there is the check for buttonView.isPressed()
, so the infinite recursion is going on as long as you hold your finger on the screen. So that code must be reworked... // EDIT: Aha, calling setChecked on the MaterialAboutSwitchItem does not trigger the checked listener, it just sets the internal value, that's why there is no infinite recursion. But still there are problems with current implementation as when you keep tapping really fast on the "This switch cannot be switched off" switch, you can switch it off.
@daniel-stoneuk I pushed some fixes for the preferences branch.
why is there areContentsTheSame method that calls getDetailString() to create big string representations of the objects and then compares the two strings? Why not just properly implement equals() method in first place?
Yep spot on with the edit, and you're right. We should implement a function to compare just the visuals - generating the strings and then comparing them is probably quite slow and wasteful. I've created issue #91. Probably won't get round to fixing this until after my University interview in a couple weeks.
The performance issues I was noticing were during scrolling. Lots of frames were dropped, which doesn't really give a good user experience. Because of this, I didn't clean up the PR too much & kept it in a separate branch. I think part of this is due to the inherent performance issues that come with using nested RecyclerViews - I knew from the start that this wouldn't be a great idea for scalability, however it seemed most practical & I didn't want to include library like groupie, which might struggle to display a cardview beneath the items. Another perk of the nested RVs is the ability to use custom adapters like LicenseAdapter.
Would you say the fixes you've made are ready to be merged? Thanks very much for the commits - as I mentioned, I don't have much free time right now but in a couple weeks should be able to put more work into it if required.
@daniel-stoneuk As I now tried it, the bad performance during scrolling really seems to be caused by the nested RecyclerViews - it can be seen when you scroll down slowly, when the Custom Adapter should be drawn, it lags for some hundreds of milliseconds. So problem is not in the checkbox/toggle.
I think the preferences branch could be merged (as it is not the cause of bad scrolling performance), but the feature should be improved in the future - e.g. configure on click listener to whole row so it will change checkbox/toggle when clicking anywhere on the row. Or also I think changing the row checked state programatically won't work. But that is related only to new 2 items, everything in library should work same as before, that why I think you can merge it.
Hmm, maybe the scrolling issues are also caused by the DiffUtil, as it seems it freezes a bit for me always when new card should be added/drawn for first time during scrolling. It should be tested, where the issue really is, but that should be new separate issue as it is not related to this issue.
Hi! Is it possible to set a toggle state programmatically?
It looks like
MaterialAboutToggleItem
doesn't have such method.I have a case when I should set toggle back depending on user answer. Like if user toggles “set fullscreen”, then we show him permission dialog and if the user denies to give the permission, we uncheck the toggle back programmatically.