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

Create MaterialAboutToggleItem #77

Open
Gaket opened this issue Feb 23, 2018 · 10 comments
Open

Create MaterialAboutToggleItem #77

Gaket opened this issue Feb 23, 2018 · 10 comments
Assignees

Comments

@Gaket
Copy link

Gaket commented Feb 23, 2018

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.

@daniel-stoneuk
Copy link
Owner

daniel-stoneuk commented Feb 24, 2018

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.

@daniel-stoneuk daniel-stoneuk self-assigned this Feb 24, 2018
@daniel-stoneuk daniel-stoneuk changed the title Set MaterialAboutToggleItem state programmatically Create MaterialAboutToggleItem May 6, 2018
@daniel-stoneuk
Copy link
Owner

Hi, this functionality has been implemented in a fork at by @filol here: https://github.com/filol/material-preference-library

@filol filol mentioned this issue May 6, 2018
@Robyer
Copy link
Collaborator

Robyer commented Nov 23, 2018

@daniel-stoneuk Perhaps you can close this issue as it was merged?

@daniel-stoneuk
Copy link
Owner

@Robyer we haven't merged it into the main branch yet since it's performance is pretty terrible.

@Robyer
Copy link
Collaborator

Robyer commented Nov 25, 2018

@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?

@Robyer
Copy link
Collaborator

Robyer commented Nov 25, 2018

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.

@Robyer
Copy link
Collaborator

Robyer commented Nov 25, 2018

@daniel-stoneuk I pushed some fixes for the preferences branch.

@daniel-stoneuk
Copy link
Owner

daniel-stoneuk commented Nov 25, 2018

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.

@Robyer
Copy link
Collaborator

Robyer commented Nov 25, 2018

@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.

@Robyer
Copy link
Collaborator

Robyer commented Nov 25, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants