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

feat(android): Adds extended support for color customization of Ti.UI.Android.SearchView #10719

Closed
wants to merge 2 commits into from

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Feb 21, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-26822

Description:
Adds a few new properties for color customization of Ti.UI.Android.SearchView.

Test case:
app.js

var window = Ti.UI.createWindow({
		title: 'Search Test',
		layout: 'vertical'
	}),
	searchViewActionBar = Ti.UI.Android.createSearchView({
		hintText: 'Action Bar Search',
		hintTextColor: 'cyan',
		iconSearchColor: 'magenta',
		iconSearchHintColor: 'yellow'
	}),
	searchViewIconified = Ti.UI.Android.createSearchView({
		hintText: 'Action Bar Default',
		hintTextColor: 'blue',
		iconSearchColor: 'green',
		iconSearchHintColor: 'red',
		iconSearchCloseColor: 'orange'
	}),
	searchViewNonIconified = Ti.UI.Android.createSearchView({
		hintText: 'Action Bar Non Iconified',
		iconifiedByDefault: false,
		hintTextColor: 'red',
		iconSearchColor: 'green',
		iconSearchHintColor: 'blue',
		iconSearchCloseColor: 'purple'
	});

window.activity.onCreateOptionsMenu = function(e) {
	e.menu.add({
		title: 'Search',
		actionView: searchViewActionBar,
		icon: Ti.Android.R.drawable.ic_menu_search,
		showAsAction: Ti.Android.SHOW_AS_ACTION_IF_ROOM | Ti.Android.SHOW_AS_ACTION_COLLAPSE_ACTION_VIEW,
	});
};
window.add([searchViewIconified, searchViewNonIconified]);
window.open();

@ypbnv ypbnv added this to the 8.1.0 milestone Feb 21, 2019
@build build requested review from a team February 21, 2019 17:16
@build build added the docs label Feb 21, 2019
@build

This comment has been minimized.

* @module.api
*/
public static final String PROPERTY_ICON_SEARCH_HINT_COLOR = "iconSearchHintColor";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the property names, perhaps we should rename them:

  • "iconSearchColor" -> "iconSearchTintColor"
  • "iconSearchCloseColor" -> "iconSearchCloseTintColor"
  • "iconSearchHintColor" -> "iconSearchHintTintColor"

Notice that I added the word "Tint" to the above properties.
This is for consistency with our other tint color properties such as:

  • ProgressBar.trackTintColor
  • Window.navTintColor
  • TabGroup.unselectedItemTintColor
  • TabGroup.activeTabIconTint(This one is missing the word "Color".)

I'm also contemplating if the word "Search" is necessary in the property names since all properties belonging to SearchView is search related. For example, a property named closeIconTintColor may be good enough.

@garymathews, do you have an opinion on the property names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the proposed name changes and I will update them.

{
try {
int id = TiRHelper.getResource("id.search_src_text");
EditText text = (EditText) searchView.findViewById(id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Member variable searchView can be null when this object's release() method gets called. So, we need to do a null check in all update methods.

Also, I'm not sure how I feel about blindly casting the reference returned by findViewById() to EditText. We could run into a class cast exception if the support library changes the implementation in the future. I'm thinking that this is likely to happen once we add gradle support and devs can have better control of the support library version they're using. Perhaps changing the catch() block below to catch Exception instead of ResourceNotFoundException would be better.

try {
searchIconDrawable = TiApplication.getInstance().getResources().getDrawable(
TiRHelper.getResource("drawable.abc_ic_search_api_material"), null);
searchIconDrawable.setColorFilter(Color.RED, PorterDuff.Mode.SRC_IN);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to call searchIconDrawable.mutate() since you're receiving a global reference to the drawable here.

ssb.append(getProxy().getProperty(TiC.PROPERTY_HINT_TEXT).toString());
}
// Add it as a hint.
editText.setHint(ssb);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ The above makes me nervous. It makes assumptions about the internal implementation details on Google's end, which could change in the future. Is there really no other nice way to do this? I was able to replace this icon via a theme/style, except when the SearchView was embedded within the ActionBar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you might be right. There might be no other way to do this without hacks... but we then have to be concerned about potential breakage/issues in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I tested providing a custom theme with overridden properties for close, hintIcon and searchIcon worked just fine. The case where I embedded the SearchView as a Menu item in the ActionBar required to separately pass the drawable I used for <item name="searchIcon">@drawable/pictureduo</item> because it is different from the already embedded resources which are referenced by ID (like the sample icon: Ti.Android.R.drawable.ic_menu_search,)
Possibly the biggest benefit would be that we can have partial parity with the properties of Ti.UI.SearchBar, but that leaves us at "Is it worth it to have that at the price of blindly assuming what is happening under the AppCompat library?" Maybe we can discuss this in details offline ?

@sgtcoolguy sgtcoolguy modified the milestones: 8.1.0, 8.2.0 Jun 3, 2019
@build
Copy link
Contributor

build commented Jul 8, 2019

Warnings
⚠️

Commit 895e42752bf9ca9a27b0cef7ac408dd3acc03edd has a message "feat(android): Adds extended support for color customization of Ti.UI.Android.SearchView" giving 2 errors:

  • header must not be longer than 72 characters, current length is 88
  • subject must not be sentence-case, start-case, pascal-case, upper-case
Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3701 tests are passing.
(There are 470 tests skipped)

📖

🚨 This PR has one or more commits with warnings/errors for commit messages not matching our configuration. You may want to squash merge this PR and edit the message to match our conventions, or ask the original developer to modify their history.

Generated by 🚫 dangerJS against b170d17

@jquick-axway jquick-axway modified the milestones: 8.2.0, 8.3.0 Jul 10, 2019
@sgtcoolguy sgtcoolguy removed this from the 8.3.0 milestone Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants