Fix issues with icons in DirectionsFragment

[AcharyaS97]

Simple fixes for issues seen in DirectionsFragment having to do with the Swap and Favorite Trip Icons

Only show the "Favourite Trip" and "Swap" icons when both LocationViews are populated.

Refer to Issue #732

[cla-bot]

Thank you for your pull request and welcome to our community! We require contributors to sign our Contributor License Agreement, and we don't seem to have the user @AcharyaS97 on file. In order for your code to get reviewed and merged, please explicitly state that you accept the agreement. Alternatively, you can add a commit that adds yourself to https://github.com/grote/Transportr/blob/master/.clabot

[ialokim]

Thanks for your PR! I'd say the conceptually cleaner way would be to not rely on the LocationView's getLocation(), but add a LiveData Transformation inside the ViewModel and only observe and react to the changes inside the Fragment.

[grote]

The Favourite Trip Icon does not relate to the locations, but to the displayed trips and should only be displayed when trips are displayed.

[grote]

I vaguely remember some design guidelines that say you should disable icons rather then hide them when they are not available.

[ialokim]

The Favourite Trip Icon does not relate to the locations, but to the displayed trips and should only be displayed when trips are displayed.

Then it might make sense to hide the trip results when the from or to field is cleared?

[grote]

Maybe yeah. I think I might have left it until the next search in case the user still needs it.

[ialokim]

I vaguely remember some design guidelines that say you should disable icons rather then hide them when they are not available.

Is there a nice-looking and understandable way of disabling borderless icon buttons in the app top bar?

[AcharyaS97]

Thank you for all the feedback! Addressing specific points,

Thanks for your PR! I'd say the conceptually cleaner way would be to not rely on the LocationView's getLocation(), but add a LiveData Transformation inside the ViewModel and only observe and react to the changes inside the Fragment.

Sure, so instead of putting the ui state change code into the from/to location observables, create a new observable for the state of the swap icon that derives its value from the state of the from/to observables in the viewmodel through a Transformation? What's the Transformation of types that would be occurring?

The Favourite Trip Icon does not relate to the locations, but to the displayed trips and should only be displayed when trips are displayed.

Right ok. So I should tie the state of that Favourite Icon to when the trips member in the ViewModel is updated?

Then it might make sense to hide the trip results when the from or to field is cleared?

In my opinion as a user, this would be fine to leave as is in case it was cleared by accident or if the user still needed the result of that last trip query to decide on what their next query would be?

[ialokim]

Sure, so instead of putting the ui state change code into the from/to location observables, create a new observable for the state of the swap icon that derives its value from the state of the from/to observables in the viewmodel through a Transformation? What's the Transformation of types that would be occurring?

I'd say the location types (from/to) -> boolean (show or hide swap icons).

So I should tie the state of that Favourite Icon to when the trips member in the ViewModel is updated?

I think this is already how it is done currently.

[AcharyaS97]

TransportrClip.mp4

I've updated it to be more idiomatic as requested. Also about disabling the icon instead of hiding it, refer to the video clip. As per material design guidelines, disabled icons should have an opacity of 38% of the regular icon if it is black. Because the starting icon is white, I went the other way, setting the disabled color to #999999 compared to the #FFFFFF of the enabled variant. If you like I can apply the same styling to the other elements as well?

[ialokim]

Thanks for your continuous work! To be honest, I'm still not really convinced by the disabled icon look. This grayish color looks a bit odd and unmotivated to me. Not sure whether @grote has another opinion or an idea on how to make this more understandable and perhaps nicer looking?

[AcharyaS97]

I've been playing around with it. What if the icon itself was changed to something like swap_vertical_circle, since it has more surface area, and the opacity was reduced on disable.

[grote]

I agree that the first version doesn't look nice. The second is already better, but ideally we wouldn't need to change the icon.

Also, did someone check how normal action bar icons look when they are disabled? Maybe we can re-use that style somehow?

[AcharyaS97]

I was thinking that the new one might be easier to see when disabled compared to the current icon, but I'll change that back if you like. Also, did you mean a style that's used in the app elsewhere, or the general design guideline for styling of a disabled icon in an action bar?

Also, final thing but is the behaviour in the clip working as expected? If both from/to are populated and the back button is pressed, should they be cleared as well?

Android.Emulator.-.Pixel_3a_XL_API_29_5554.2020-12-31.17-28-36.mp4