Closed radass1996 closed 4 years ago
I left some comments already, but could not do a full review right now. Anyhow thanks a lot for your contribution so far, I'll take a closer look in the next few days.
@ramack We can also let Logout option is settings and make another one in navigation drawer. Or logout option shoulb be ONLYin navigation drawer ?
@ramack Still struggling little bit with default album. Can you please send me a link for some official Piwigo Android Api documentation ?
@ramack Can we please split other Setting functionalities to other Pull Requests ? If you will be not busy just check code leave me comments and i will fix them. Thanks for support :) .
@ramack User will be not able to remove last account. Only way will be proper logout. What you think ?
@ramack added number row setting property
@ramack We can also let Logout option is settings and make another one in navigation drawer. Or logout option shoulb be ONLYin navigation drawer ?
I think it is sufficient to have it in one place. In the iOS app all the account-related things are in the settings, so it is fitting that there is also a logout option, in Piwigo-Android I don't really see the point why we should have a logout in the settings, as "logout" it not really a setting, is it?
@ramack Still struggling little bit with default album. Can you please send me a link for some official Piwigo Android Api documentation ?
You can check the REST API in a piwigo installation. For example in the demo: http://piwigo.org/demo/tools/ws.htm
But: I don't think it is related, as the default album should be just the album opened by the App as default without affecting piwigo on server side at all.
@ramack Can we please split other Setting functionalities to other Pull Requests ? If you will be not busy just check code leave me comments and i will fix them. Thanks for support :) .
Yes for sure, that is why I proposed to even just care for the general activity and the mechanisms to store and restore the settings (for global but also for account-related one) in issue #44 and why I asked you to start "small".
@ramack User will be not able to remove last account. Only way will be proper logout. What you think ?
as written above: It's not very nice, and worth documenting it in an issue. If you want you are welcome to work on that also (and I think it makes sense, as you have started that already) but it should be in a separate PR. On the other side: about your logout code: wouldn't it be enough to
What do you think?
@radass1996 I stumbled over PreferencesRepository - which was created by the initial contributor of the apps code @Philio. Currently it is not really used, as only the current selected account is written into the DefaultSharedPreferences which is done via PreferencesRepository.setActiveAccount.
Nevertheless this PreferenceRepository seems like a good candidate to manage the settings and ensure that those which are account specific are stored in the accounts data and the generic settings in the DefaultSharedPreferences, while the users don't need to distinguish for that and can just query the PreferenceRepository. And we can even change the location of the storage without affecting the code which rely on (=uses the settings value) and the PreferenceRepository can be injected via DI in many places without further code.
Means: it would be great to use that and further develop it. What do you think?
@ramack Please check fixed comments.
@radass1996 I stumbled over PreferencesRepository - which was created by the initial contributor of the apps code @Philio. Currently it is not really used, as only the current selected account is written into the DefaultSharedPreferences which is done via PreferencesRepository.setActiveAccount.
Nevertheless this PreferenceRepository seems like a good candidate to manage the settings and ensure that those which are account specific are stored in the accounts data and the generic settings in the DefaultSharedPreferences, while the users don't need to distinguish for that and can just query the PreferenceRepository. And we can even change the location of the storage without affecting the code which rely on (=uses the settings value) and the PreferenceRepository can be injected via DI in many places without further code.
Means: it would be great to use that and further develop it. What do you think?
@ramack Preference repository Injected :) Please check the review.
Great, this looks close to being merged. Thanks for all your efforts! I left some more minor comments and beside those I think you should
- add yourself in strings.xml/contributors
- if possible enhance the PreferencesRepository to store not everything in SharedPreferences, but some settings in the Account. The list of image sizes is for example configurable in Piwigo on server side, so this one would be great to store dependent on the current account and check it against available_sizes in the StatusResponse
@ramack , thanks for support, on second point i need more time to think. Can i add it in next PR ?
@ramack Check some fixed comments please :)
Great, this looks close to being merged. Thanks for all your efforts! I left some more minor comments and beside those I think you should
- add yourself in strings.xml/contributors
- if possible enhance the PreferencesRepository to store not everything in SharedPreferences, but some settings in the Account. The list of image sizes is for example configurable in Piwigo on server side, so this one would be great to store dependent on the current account and check it against available_sizes in the StatusResponse
@ramack , thanks for support, on second point i need more time to think. Can i add it in next PR ?
Yes for sure, that is a reasonable point to split!
@ramack So, can we merge PR :) ?
when I tried to logout I had it that the complete account was unexpectedly deleted...
when I tried to logout I had it that the complete account was unexpectedly deleted...
But account should be deleted from app during Logout. Or not ?
a few more things poped up, especially for the deprecated APIs we need to improve, because it is not good to start with deprecated stuff... Also the summaries are quite short and should not just reflect the pure selections, but an explaining text like "Photo size 'xxsmall' will be used for download." or "5 photos shown per row".
edit: and the "back" icon is different in the settings activity. You probably should use the getSupportActionBar().setDisplayHomeAsUpEnabled(true);
@ramack OK
@ramack Check fixed comments pls
when I tried to logout I had it that the complete account was unexpectedly deleted...
But account should be deleted from app during Logout. Or not ? I think not, then it would be delete account, which is what the accountmanager is for... like this I think it is a bit dangerous to loose the account settings by accident.
@ramack Now we not remove account during LOGOUT. But after trying to log in again to the app. I realized that is not possible. Do you will manage this in different issue ?
@ramack Please check review and uninstall old app before you install new one(Shared prefferences)
@ramack Now we not remove account during LOGOUT. But after trying to log in again to the app. I realized that is not possible. Do you will manage this in different issue ?
we could do that, yes. But what was the original problem why you added the logout? (Sorry, I forgot) For me it looks like after a Logout we should not open a LoginActivity (which has a bit a misleading name, actually it is a "CreateAccountActivity" - also not fully true, because it doesn't create an account on piwigo server side) After a Login - if there still are accounts configured (which is now always the case) we should more show the ManageAccountActivity to select a new one.
But maybe the real issue was just, that on account deletion in the AccountManager we still keep the last login (if no other account is selected), so maybe the best fix for that would be to have a logout called from there if the last account was deleted and there show the LoginActivity again to create a new one.
@ramack Now we not remove account during LOGOUT. But after trying to log in again to the app. I realized that is not possible. Do you will manage this in different issue ?
we could do that, yes. But what was the original problem why you added the logout? (Sorry, I forgot) For me it looks like after a Logout we should not open a LoginActivity (which has a bit a misleading name, actually it is a "CreateAccountActivity" - also not fully true, because it doesn't create an account on piwigo server side) After a Login - if there still are accounts configured (which is now always the case) we should more show the ManageAccountActivity to select a new one.
But maybe the real issue was just, that on account deletion in the AccountManager we still keep the last login (if no other account is selected), so maybe the best fix for that would be to have a logout called from there if the last account was deleted and there show the LoginActivity again to create a new one.
- In a sense I think the "delete last account" use case is not very common, we should maybe not "waste" an entry in the navigation drawer for that...
@ramack So, i will try to implement Seekbar. What else should i change ? I added logout here because i thought that it will be part of settings(because of iOS screens). Then we moved logout to Nav Drawer.
@ramack Here is Seekbar commit. I dont get what you exactly want regarding logout stuff. Please check comments.
close by mistake
For the logout I have mainly the question why we need it.
For the logout I have mainly the question why we need it.
Shouldn't we replace it with some "Change account.." or something ?
We might get rid at some point of the "Sandwich" side menu and go for a settings icon like on iOS
For the logout I have mainly the question why we need it.
@ramack Should i remove whole Logout ??
Thanks a lot for the effort you have put into this. With respect to the settings I had nothing to add, so I just merged it. You really did a great job and I am sorry, that my comments sometimes have been misleading and were not always clear.
For the logout I think it was not really clean to integrate the logout functionality in this PR, normally it is better to have a separate PR for different functionalities. Even in case are have a sequential dependency, it is reasonable to make two PRs. One of the reasons is, that they are small by this and this makes it easier to review, but also to track down an issue in case it turns out that it introduces a bug.
I remember that you had a valid reason why you added the logout, but I don't find it anymore. If you remember I would propose, that you create a new issue describing that problem and we can discuss there how that specific problem is solved best. And after that cleanup what is necessary. Today I am not in a position to clearly see the pros and cons of the Logout feature as it is right now.
@ramack Thanks for your cooperation and support.
@ramack Implemented setting options are brightness and thumbnail size. Added Logout.