Feature: reminder type selector

[maxcutlyp]

Implementing #30.

[maxcutlyp]

Right now the button doesn't do anything other than open up the three options. Placeholder icons and colours are used (although I don't mind the current menu button colours).

[felixwiemuth]

Thanks for getting started on this!

It looks already very good! I know it's only a POC, but as it already looks close to a usable version, here are some comments:

• The colors look good and fit the design!
• With the current positioning, it might even work without any blurring. Without the labels, there is nearly no unpleasant overlap with other UI elements. Maybe the labels could get some blurring margin. And we could try adding depth (it's called elevation in Android; a first attempt of just adding 2dp-16dp with the elevation attribute to the menu FABs didn't seem to make any difference though).
• The main button looks a bit too big for my taste (I noticed this directly when seeing it for the first time).
• The animation on the main button looks rather distracting, an animation for opening the menu should be enough. Are there different opening animations for the menu buttons? The current one even seems a bit too wild for me; the zoom and extension seems nice, but the "popping up" of the individual menu items seems unnecessary.
• There seems to be an invisible margin which cuts the menu buttons slightly on the right side.

Btw, Github has Draft pull requests, I converted it to one.

[maxcutlyp]

The colors look good and fit the design!

Thanks!

With the current positioning, it might even work without any blurring. Without the labels, there is nearly no unpleasant overlap with other UI elements. Maybe the labels could get some blurring margin. And we could try adding depth (it's called elevation in Android; a first attempt of just adding 2dp-16dp with the elevation attribute to the menu FABs didn't seem to make any difference though).

Okay, I'll look into it. Strange that adding the elevation attribute didn't do anything, since they use it all over the example app.

The main button looks a bit too big for my taste (I noticed this directly when seeing it for the first time).

Have you looked at it on a physical device (with wireless debugging, for example)? It's the "mini" version, so it's the same size as the menu buttons. The only other option (without scale attributes) is the "normal" version, which is the bigger size in the example gif. In my opinion, if it gets any smaller it could run the risk of being too small for some users to consistently tap.

The animation on the main button looks rather distracting, an animation for opening the menu should be enough.

The spin? I agree. It can be removed by setting the menu_anim_button attribute to false.

Are there different opening animations for the menu buttons? The current one even seems a bit too wild for me; the zoom and extension seems nice, but the "popping up" of the individual menu items seems unnecessary.

Unfortunately there is only one opening animation available in the AFAB project. The "popping up" effect is caused by the interpolator they have chosen here: overshoot. Looking at the source code reveals that there's no built-in way to specify a different animation - it's set here and ctrl+F shows that it is not changed again.

The closing animation uses the acceleration_quint interpolator, which works well for the opening animation too, but making that change would mean cloning the project and using it as some type of local dependency. I'm not sure what the process looks like to be honest, but I'm sure it's not too difficult - we can go that route if you'd like. Another option would be submitting a pull request to the original project with an attribute to change the animation (or even just the interpolator) but considering it hasn't been modified in over two years, I wouldn't hold my breath waiting for it to be merged.

There seems to be an invisible margin which cuts the menu buttons slightly on the right side.

You're right, I didn't notice it before. It can be fixed by adding an paddingRight attribute set to 10dp, which is enough to cover the "pop out" animation as well as the final size.

Thanks for converting it to a draft, I must have missed the option to create it as one.

[maxcutlyp]

The above commit removes the spin animation and fixes the margin issue, in addition to what was listed in the commit message.

[felixwiemuth]

The main button looks a bit too big for my taste (I noticed this directly when seeing it for the first time).

Have you looked at it on a physical device (with wireless debugging, for example)? It's the "mini" version, so it's the same size as the menu buttons. The only other option (without scale attributes) is the "normal" version, which is the bigger size in the example gif. In my opinion, if it gets any smaller it could run the risk of being too small for some users to consistently tap.

Yes, my impression came from a physical device. But it's also more about the relative size in relation to the other UI elements (text field, date buttons, add button).
Of course there is no one size that fits everyone, some want it neat and efficient, some need bigger UI elements to be able to comfortably use it.

I don't think that a slightly smaller button, better fitting into the remaining UI, would be too small compared to other UI elements commonly found. In addition, in the system settings one can set the general size for UI elements, so that if one has problems of comfortably using the UI, one can scale it up. I just tested it with the Add Reminder dialog and it works fine. So I I'd prefer to try a smaller version - it must be possible to set the size freely somewhere, I would expect in the XML?

Unfortunately there is only one opening animation available in the AFAB project. The "popping up" effect is caused by the interpolator they have chosen here: overshoot. Looking at the source code reveals that there's no built-in way to specify a different animation - it's set here and ctrl+F shows that it is not changed again.

The closing animation uses the acceleration_quint interpolator, which works well for the opening animation too, but making that change would mean cloning the project and using it as some type of local dependency. I'm not sure what the process looks like to be honest, but I'm sure it's not too difficult - we can go that route if you'd like. Another option would be submitting a pull request to the original project with an attribute to change the animation (or even just the interpolator) but considering it hasn't been modified in over two years, I wouldn't hold my breath waiting for it to be merged.

Thanks for tracking it down and good if it doesn't seem too difficult to change.

It's easy to make changes (which we might have to/want to anyway if the project is no longer maintained) and still include it as a dependency: we would simply fork the project on Github and then include it via jitpack in the same way as of now, or add it as a submodule, like it is done with SectionedRecyclerViewAdapter (which will be removed though). The latter makes local development a bit easier: one can simply work in the local copy of the dependency (and even commit and push changes from there) instead of temporarily changing the dependency to another local project.

As these changes are not crucial for now, we could wait until everything else is sorted out. But if you want to experiment, you can of course go ahead and fork the Advanced Floating Action Button project. When you add it as a submodule you will set and then later update the commit that the main repository is supposed to use. If for instance you want to use a specific branch from the forked AFAB project, that's no problem. Probably this intro on git submodules and maybe a bit of the git book should be enough to get you going.

The above commit removes the spin animation and fixes the margin issue, in addition to what was listed in the commit message.

Sounds good! I'll take a look soon.

[maxcutlyp]

Yes, my impression came from a physical device. But it's also more about the relative size in relation to the other UI elements (text field, date buttons, add button).
Of course there is no one size that fits everyone, some want it neat and efficient, some need bigger UI elements to be able to comfortably use it.

I don't think that a slightly smaller button, better fitting into the remaining UI, would be too small compared to other UI elements commonly found. In addition, in the system settings one can set the general size for UI elements, so that if one has problems of comfortably using the UI, one can scale it up. I just tested it with the Add Reminder dialog and it works fine. So I I'd prefer to try a smaller version - it must be possible to set the size freely somewhere, I would expect in the XML?

Okay, sure. I made it slightly smaller, so it's approximately the same size as the circular indicator on the date picker now.

It's easy to make changes (which we might have to/want to anyway if the project is no longer maintained) and still include it as a dependency: we would simply fork the project on Github and then include it via jitpack in the same way as of now, or add it as a submodule, like it is done with SectionedRecyclerViewAdapter (which will be removed though). The latter makes local development a bit easier: one can simply work in the local copy of the dependency (and even commit and push changes from there) instead of temporarily changing the dependency to another local project.

I think the latter is the way to go. There's a few other things I'd like to change the AFAB source code for (exposing some functions, mostly) so I'll look into it.

When you add it as a submodule you will set and then later update the commit that the main repository is supposed to use. If for instance you want to use a specific branch from the forked AFAB project, that's no problem. Probably this intro on git submodules and maybe a bit of the git book should be enough to get you going.

Thanks for the tips and the resources!

PS: I increased the size of the labels, which in my opinion makes them stand out more and blend into the background less. Let me know what you think.

[maxcutlyp]

Okay, I've forked the AFAB repo, added it as a submodule, and pushed some changes to it in a new branch (SimpleReminder-changes). Hopefully I did everything right - I think you'll need to run git submodule update (or maybe init, since it's a new submodule?). Let me know how it goes, this is my first time working with git submodules.

Also, I made the main button change colour according to the selection because I didn't think the selection change was obvious enough. Let me know what you think.

[felixwiemuth]

Looks good with the submodule! As the improvements you make to the project are probably not just SimpleReminder-related, you probably want to merge them into master or at least some general feature branch there before we merge here, and then you can make a pull request to the main project (just to see whether there is interest; if it gets merged, we can switch back to the main project).

The button size seems fine now (even though it could still be a little bit smaller, but that will also depend on which icons we choose). However, I would reduce the margins to the left and right, so that it is more above the "+" button and the text goes nearly up to the button.

I'm not sure which size of the labels I like best, let's decide when we have a near final version.

Very nice with the current choice now being indicated by main button in a grey version as well as in the menu! With real icons it will probably look very good!

As commented in the code, let's keep the data model as-is for now and focus on the UI. Currently there are no alarm reminders anyway, so we don't need any change in the data model. We could use the new UI even before alarm reminders are available, the corresponding button could just result in a toast "Coming soon: alarm reminders" :).

So maybe the long-click actions as well as removal/change of animation would be next?

Good work!

[felixwiemuth]

Is it possible to specify the size in dp, so it's more in line with the size specification of other UI elements? Or, if the original is in dp, then it's maybe okay. I just want to make sure it scales fine in relation to the other UI elements for different screen sizes.

[maxcutlyp]

The scaleX and scaleY parameters are a percentage, so 0.95 means 95% of the original size (see the docs). The original, unscaled sizes are also specified in dp.

[felixwiemuth]

Okay then it's fine, especially if it is not possible to specify dp here.

[felixwiemuth]

Regarding changes to persistency (for example in this class), let's rather not continue on this for now because I haven't thought yet about how to do this best. (Several things have to be considered here including compatibility with the previous data format, whether it's "future save" and also that this class and others are now implemented in Kotlin.)

[maxcutlyp]

As the improvements you make to the project are probably not just SimpleReminder-related, you probably want to merge them into master or at least some general feature branch there before we merge here, and then you can make a pull request to the main project (just to see whether there is interest; if it gets merged, we can switch back to the main project).

My only concern with that is that some changes we make will only be needed for the app - for example, changing the interpolator. I'm not sure that implementing a way for users of the library to change the interpolator without modifying the source code is within the scope of this PR, but simply changing that one line of source code is. The only other changes I've made is exposing some getters on the button colours and making the setColors function not internal so it can be called from outside code (see this diff) and I'm not sure if that warrants a whole PR.

The button size seems fine now (even though it could still be a little bit smaller, but that will also depend on which icons we choose). However, I would reduce the margins to the left and right, so that it is more above the "+" button and the text goes nearly up to the button.

Good idea, I'll do that.

I'm not sure which size of the labels I like best, let's decide when we have a near final version.

Sure.

Very nice with the current choice now being indicated by main button in a grey version as well as in the menu! With real icons it will probably look very good!

Thanks!

As commented in the code, let's keep the data model as-is for now and focus on the UI. Currently there are no alarm reminders anyway, so we don't need any change in the data model. We could use the new UI even before alarm reminders are available, the corresponding button could just result in a toast "Coming soon: alarm reminders" :).

Okay, sure. That would mean it wouldn't be possible to remember the reminder type for alarm reminders, which I guess is not that big of an issue since they haven't been implemented yet.

So maybe the long-click actions as well as removal/change of animation would be next?

Yep, I'll get on that soon too.

[felixwiemuth]

My only concern with that is that some changes we make will only be needed for the app

Ah okay, that's also fine for now.

Another aspect to consider: Does the AFAB library have further dependencies and are they maintained? Is the code generally fine/are there any important Lint warnings if you run code inspection on the project?

That would mean it wouldn't be possible to remember the reminder type for alarm reminders, which I guess is not that big of an issue since they haven't been implemented yet.

Exactly; one shouldn't even be able to choose the alarm reminder type (optimally, selecting it will only show the toast, not change the choice). And when one then for instance edits a reminder, the selection should be at either normal reminder or nagging reminder.

So maybe the long-click actions as well as removal/change of animation would be next?

Yep, I'll get on that soon too.

Sounds good, thanks!

[felixwiemuth]

Luckily this will anyway be included because of the switch to Kotlin (I'll try to push my maintenance work soon).

[maxcutlyp]

Does the AFAB library have further dependencies and are they maintained?

From AFAB's build.gradle:

implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version" // 1.3.41
implementation 'androidx.appcompat:appcompat:1.1.0'
implementation 'androidx.core:core-ktx:1.1.0'
testImplementation 'junit:junit:4.12'
androidTestImplementation 'androidx.test:runner:1.2.0'
androidTestImplementation 'androidx.test.espresso:espresso-core:3.2.0'

All of the above libraries have new versions available, although they are all on their most recent major versions. The first three are widely used and supported, and the last three are test dependencies which are basically unused, given that the two tests in the library (1, 2) are examples that haven't been touched since the first commit.

Is the code generally fine/are there any important Lint warnings if you run code inspection on the project?

There are a few (11) lint warnings but they don't seem too important. Two of them are labelled as errors and they both seem relatively simple to fix:

/.../fabmenus/fabmenu/src/main/java/com/kelmer/android/fabmenu/fab/FloatingActionButton.kt:32: Error: This custom view should extend android.support.v7.widget.AppCompatImageButton instead [AppCompatCustomView]
) : ImageButton(context, attrs, defStyleAttr), Checkable {
    ~~~~~~~~~~~
/.../fabmenus/fabmenu/src/main/java/com/kelmer/android/fabmenu/fab/Label.kt:22: Error: This custom view should extend android.support.v7.widget.AppCompatTextView instead [AppCompatCustomView]
) : TextView(context, attrs, defStyleAttr) {
    ~~~~~~~~

I can try to fix them if you'd like, but they haven't affected anything so far.

There is also an error in one of the tests where ext on line 4 is an unresolved reference. Since this code is never used and was probably part of a template (as mentioned earlier), it hasn't affected anything until I searched it out just now.

Exactly; one shouldn't even be able to choose the alarm reminder type (optimally, selecting it will only show the toast, not change the choice). And when one then for instance edits a reminder, the selection should be at either normal reminder or nagging reminder.

Done in the commits above (50bc888 and 42e789f) :)

[maxcutlyp]

It now uses the accelerate_quint interpolator instead of overshoot - feel free to experiment with other interpolators and choose the one you like the best, otherwise we can leave it as accelerate_quint, which is the interpolator used for the closing animation. Keep in mind you'll probably have to run git submodule update.

The FAB is also now aligned with the "+" button.

[maxcutlyp]

Long clicking is implemented now too, on the menu icons and on the main icon (acting as whichever is currently selected).

[felixwiemuth]

Regarding the AFAB library

Thanks for checking the code and dependencies. I suggest

• Remove the tests if they are useless (and the dependencies)
• Update all dependencies to the newest version (if that gives errors, we can do that after the maintenance updates in the SR project itself)
• Fix the lint errors (if it's quick, otherwise let me know)
• Also update gradle to the newest version

Then see whether everything still works fine.

We just have to keep in mind that by making our UI depend on this library, we will have to maintain it to some degree and we don't want that to be troublesome.

Regarding the updates

• Alignment looks good now
• Long-click actions seem to work as expected, good job!
• Animation: it doesn't look that different on first sight, but comparing I might even find the previous one better. It would be nice if there was something more simple... Can one change the speed? I think I'd prefer it to open a bit faster, maybe then it also looks different. I also prefer the version where main button and labels do not have light/flashing effects when clicking.

[maxcutlyp]

I suggest

• Remove the tests if they are useless (and the dependencies)
• Update all dependencies to the newest version (if that gives errors, we can do that after the maintenance updates in the SR project itself)
• Fix the lint errors (if it's quick, otherwise let me know)
• Also update gradle to the newest version

Then see whether everything still works fine.

Sure, I'll look into it and let you know how it goes.

Animation: it doesn't look that different on first sight, but comparing I might even find the previous one better. It would be nice if there was something more simple... Can one change the speed? I think I'd prefer it to open a bit faster, maybe then it also looks different.

The duration property in fabmenus/fabmenu/src/main/res/anim/fab_scale_up.xml can be changed - it was previously set to 200ms. There's also a bunch of different interpolators to choose from - Android Studio shows an autocomplete list when I delete the accelerate_quint part (so it looks like @android:interpolator/) and hit Ctrl+Space. I think accelerate_decelerate at 100ms looks good (which is what I've changed it to for now) but it's mostly just personal preference so feel free to test out others.

I also prefer the version where main button and labels do not have light/flashing effects when clicking.

Disabling the "flashing" effect on the labels is pretty straightforward, it just involves setting menu_labels_colorPressed and menu_labels_colorRipple to @color/fab_label_normal (i.e. the default label colour).

Doing the same for the main button is a bit harder because it changes colour depending on if it's open or closed, so the ripple/pressed colours would have to be changed accordingly. This is a more complex task than it seems, since the setColors function sets the currentColor variable to the colorNormal value, even if it's currently set to colorReveal. Handling this would require adding additional internal state to the FloatingActionButton class, or checking if currentColor is equal to colorReveal or the previous colorNormal which seems less than ideal.

Another option would be to add a function called something like setColorsWithoutSettingCurrentColor. This would make the user of the library responsible for handling that state, and it is the method I have chosen. There is no need to set currentColor to colorReveal because colorReveal isn't set in setColors.

A third option would be to add an attribute called something like menu_fab_doRipple, but that seems like overkill for our use case.

To quickly compare the button with and without the flashing, you can comment out lines 187 to 200 in ReminderDialogActivity.java.

PS: I also added some potential icons. The bell and alarm ones are borrowed from SystemUI (with credit in LICENSE.md) and the bell with the ! through it is a modified version of the bell icon. I think it looks alright, but the transition between the normal and nagging icons looks a bit strange because I had to move the right hand side of the bell out two units to be able to fit the ! in while still keeping some of the top and bottom bits and some of the side of the bell visible. Let me know what you think.

[felixwiemuth]

Sure, I'll look into it and let you know how it goes.

Perfect!

Regarding your updates

• The nagging icon you designed looks pretty good; I'm not sure whether the meaning is directly intuitive, but it would do its job. If you are up for it, we could think about and try some other designs, otherwise this one is good as well.
  • Do you think an exclamation mark within the bell would be too small?
  • Otherwise I could imagine just two or three dots next to each other within the bell. Here it would also make sense to have the bell a bit larger, maybe just horizontally. This would at the same time distinguish it more from the original bell symbol.
• Having the icons on the buttons, I am more convinced that an even smaller size would fit the design better. To not compromise usability, the sensitive area (which reacts to a click) could be slightly larger than the button itself. I faintly remember that this might even be an existing feature (so one doesn't have to realize it with a transparent area, which would be the other option).
• Overall the animation looks much better now!
  • It is really better without the flashing effects on main button and labels (thanks for making it work).
  • I don't think it's necessary either that the buttons from the opened menu turn grey when pressed.
  • I'll try different animation times.

As I am not into the internals of the library, I haven't really checked how you made it possible with avoiding animation on the main button (thanks for the description though). I suggest to document at least the methods you add to the library if it is not intuitive for externals (for example, I wouldn't know what the different color types like "current color" refer to). If instead a general explanation at the class level would do (describing the concepts/terms) then that would also be helpful.

Again, very good work!

P.S. I haven't looked at your code in detail yet, I will do a full review in the end where I might give some suggestions for improvement. I will probably also ask you to cleanup the commit history in the end (I don't want to squash all commits, as for example including the icons from elsewhere and adding a dependency etc. should be a separate commit, while other commits can be combined into one); I hope that will be okay for you, otherwise I could do that.

[felixwiemuth]

I have now pushed the maintenance changes I mentioned before, so this branch can be rebased onto the updated develop branch. There will be a few merge conflicts, but they are easy to solve. If you like, I would do the rebasing.

I also just tried different animation parameters, and the following times and interpolators seem best to me:

• scale up: 50ms, linear
• scale down: 100ms, accelerate_quint (that's how it was, linear doesn't look different)

Lower numbers don't seem to make it faster anyway (it probably depends on the animation speed set in system settings).
I also noticed that the "shaking" of the labels happens independently (at least it seems so) of the animation type.

Thanks again for your work, when do you think you would continue with the few things that are left?

[felixwiemuth]

@maxcutlyp are you planning to complete this pull request?

[maxcutlyp]

Hey!

I'd like to start by apologising for my disappearance, and more specifically, my lack of communication regarding it. The short is that I got a new job and found myself with significantly less spare time, and suddenly two months had passed without me realising it.

As for this PR - I would love to complete it, but to be honest, I can't promise I'll be able to. I think it's best if you finish it off - there shouldn't be much left after the merge conflicts are resolved. I'll add you as a collaborator to my fabmenus fork so you can easily make changes without the URL changing - if you'd like me to transfer it instead, let me know.

Again, sorry for my absence. Good luck with the rest of this feature - I look forward to seeing it in a future update.

[felixwiemuth]

Hi, thanks for your message and congratulations with the job. It's understandable that it got your focus.

I'll complete this pull request then, no worries.

Regarding the fork of afab, I think I will rather create a fork under my account, so that everything is in one place. I'll probably even start with a fresh fork and implement the changes a bit differently.

Regarding the removal of the ripple effect on the main button, I see that the current solution is to just set the ripple color in the onClick listener, which I guess results in that the effect cannot be seen. I think I'd really prefer a "doRipple" attribute as you suggested before or just remove the effect completely in the library's code. So I think I'll try to find the places in the code where the effect is created and make that part conditional or remove it.

Regarding your contributions, I'll add you to the list of contributors (probably also within a new section for the icon). Your entry will link to your Github profile, but you can choose a different display name instead of your user name and optionally I'll add an email address. Let me know what you prefer.

Thanks again for your help! Whenever you feel like contributing again, please let me know, I'd really appreciated it and there is always a lot to do :).