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
[TIMOB-17964] Android 5.0: Add support for Toolbar #9147
Conversation
Initial commit - adds Toolbar proxy with basic functionality.
apidoc/Titanium/Android/Activity.yml
Outdated
summary: Sets a toolbar instance to be used as an ActionBar. | ||
parameters: | ||
- name: toolbar | ||
type: Titanium.UI.Android.Toolbar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setter/getter are generated by the apidoc-generator automatically, you don't need to specify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't described that as I should. I will add a more detailed description.
The Activity class does not handle property changes. So this method is used if you want to set a Toolbar as ActionBar after the instance is created. So technically currently this is not a setter for the supportToolbar (maybe I should assign it the toolbar reference).
apidoc/Titanium/Android/Activity.yml
Outdated
@@ -435,6 +441,10 @@ properties: | |||
type: Number | |||
permission: write-only | |||
|
|||
- name: supportToolbar | |||
summary: Toolbar instance that serves as ActionBar | |||
type: Titanium.UI.Android.Toolbar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add the since: "6.2.0"
key :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. Will add it.
- name: getContentInsetEnd | ||
summary: Returns the margin at the toolbar's content end. | ||
returns: | ||
type: Number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: No manual setter / getter needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't added the content insets as properties, so these are directly setting/getting values to/from the native view. I wanted to keep the properties the same as the iOS ones (except borders) and just have Android only methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. iOS does not expose the property, and the getter / setter are generated by the build-script, so when you explicitly write them, they will likely be generated twice.
Example:
This line in the yml-files of Ti.UI.View
will produce this property, but also the getter and setter of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I got that.
There is no property "contentInsetEnd" in the Toolbar.yml. This is because there is no such property in the ToolbarProxy in Android.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty. Just thought that people would still like the contentInsetEnd
property, but let's keep it then.
@@ -93,7 +83,7 @@ public void setBigLargeIcon(Object icon) | |||
@Kroll.method @Kroll.setProperty | |||
public void setBigPicture(Object picture) | |||
{ | |||
TiDrawableReference source = makeImageSource(picture); | |||
TiDrawableReference source = ResourceHelper.getInstance().makeImageSource(this,picture); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to always space your arguments (this, picture)
@@ -0,0 +1,28 @@ | |||
package ti.modules.titanium.filesystem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should go in org.appcelerator.titanium.util
with our other helpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will create a circular dependency between 'titanium' and 'titanium-filesystem' modules.
@Kroll.method | ||
public void setSupportActionBar(TiToolbarProxy tiToolbarProxy) { | ||
TiBaseActivity activity = (TiBaseActivity) getWrappedActivity(); | ||
if (activity != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brackets
if (activity != null) {
activity.setSupportActionBar((Toolbar)tiToolbarProxy.getToolbarInstance());
}
apidoc/Titanium/Android/Activity.yml
Outdated
- name: setSupportActionBar | ||
summary: Sets a toolbar instance to be used as an ActionBar. | ||
description: | | ||
This methos is used if you want to add a Toolbar as an ActionBar after the Activity has been created. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, methos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: FAIL
@garymathews Fixed typo and code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
Since you're using "android.support.v7.widget.Toolbar" from the support library, this may mean that your new Toolbar feature is supported on Android 4.0 (API Level 14), which is the minimum API Level Titanium supports. Google's docs kind of suggest this here...
https://developer.android.com/training/appbar/setting-up.html
Can you please verify this please?
If this is true, then your doc changes no longer need to state that Android 5.0 is the min version needed anymore.
@@ -0,0 +1,28 @@ | |||
package ti.modules.titanium.filesystem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "ResourceHelper.java" file really needed? In all the places you call ResourceHelper.makeImageResource()
, you can call the TiDrawableReference.fromObject()
method and it'll provide you the same results, plus it'll support some additional image sources such as TiBlob and resource ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's way better. I didn't realize I could use it.
@@ -53,7 +56,9 @@ | |||
protected ActionBarProxy actionBarProxy; | |||
|
|||
private KrollFunction resultCallback; | |||
|
|||
|
|||
public TiToolbarProxy toolbarProxy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new "toolbarProxy" member variable doesn't appear to be accessed anywhere.
Also, I would prefer that all member variables be private or protected (preferably private) so that we can control how they're assigned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
*/ | ||
public TiToolbar(TiViewProxy proxy) { | ||
super(proxy); | ||
toolbar = new Toolbar(TiApplication.getAppCurrentActivity()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you're passing the getAppCurrentActivity() context to the Toolbar. Are you sure this is safe? Odds are this is going to be the previous activity in the stack, not the activity the toolbar will be assigned to. And I believe there is a possibility where this method can return null as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess proxy.getActivity() should guarantee us that the proper one is passed?
* @param proxies View proxies to be used | ||
*/ | ||
public void setItems(TiViewProxy[] proxies) { | ||
for (int i=0; i < proxies.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should check if the "proxies" argument is null.
Guard for passing the correct activity during creation. Guard for null parameter. Remove redundant variable.
@jquick-axway Updated the PR. Otherwise the Toolbar is working fine on every version down to API 16. Which is the minimum Android version required for SDK 6.0.0+, according to the compatabilty matrix. I haven't tested it on API 15 and 14. |
A toolbar can be positioned everywhere in the the layout as a [View](Titanium.UI.View). It can also be used as an ActionBar | ||
for [activities](Titanium.Android.Activity). This allows a toolbar to inherit [ActionBar's](Titanium.Android.ActionBar) methods, | ||
properties and events and provide a better customization of this design element. For example you can provide your own images | ||
to be used as navigation button icon, nverflow menu icon and logo. In order to do that an application must be using Theme.AppCompat.NoTitleBar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: Change "nverflow" to "overflow".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the following needs a comma. Please change...
In order to do that an application...
...to...
In order to do that, an application...
- name: extendBackground | ||
summary: If `true`, the background of the toolbar extends upwards by the height of StatusBar for current device. | ||
description: | | ||
This property is Numberended to be used when the toolbar is used as an ActionBar. It allows the user to specify |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "Numberended".
Also, the "extendBackground" Java code appears to auto-size and auto-position the toolbar. So, was this description intended for the "translucent" property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Numberended" happens when you have written "intended" and you replace all "int" to "Number" to match the documentation format. : )
'extendBackground' shouldn't be doing that - it adds padding at the top equal to the StatusBar height and makes the latter's background transparent. It will do it wherever you place your toolbar and even if it is not passed as an ActionBar. Can you provide me a snippet where that happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the "extendBackground", I was more confused about the wording in the documentation. I haven't actually tested it.
summary: Returns the source of the navigation image in the format it was set. | ||
|
||
- name: getOverflowIcon | ||
summary: Returns the source of the overflow menu image in the format it was set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the "return:" documentation for getLogo(), getNavigationIcon(), and getOverflowIcon() should be set to...
[String, Titanium.Blob, Titanium.Filesystem.File]
...like how it is for their setters.
Also, perhaps it would be better to document these as properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we document them as properties that would mean that you can access them directly and the widget is listening for their change. This is not the case with the Logo, OverflowIcon and NavigationIcon. They can be changed only through these methods. We can change them to be properties (also the content inset ones) if that will be more convenient from user standpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't have to be properties, but ideally they should be accepted as dictionary field settings in your createToolbar() function. The reason is because this is how an Alloy XML file would pass a "Toolbar" element's attributes to the createToolbar() function. And I believe most of a create function's dictionary fields are expected to be the object's properties as well.
@hansemannn, do you have have any advise regarding this?
* @param value Boolean value to set. True makes the Toolbar's background extend. False - default behaviour. | ||
*/ | ||
private void handleBackgroundExtended(boolean value) { | ||
if (value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling handleBackgroundExtended(false)
will do nothing. So, once you've set it to true, it's locked-in forever. This was the intent, right?
If so, then I would get rid of the "value" argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I have missed that.
We should also document an example on how to set up a Toolbar via Alloy and XML, like how we document it for ActionBar. This may be important for developers who wish to migrate their ActionBar Alloy app settings to the new Toolbar way of doing things. |
@jquick-axway Alloy itself will need some changes to support the Toolbar on Android. I did some adjustments, but they may not be relevant if we decide to move the widget in Ti.UI for both platforms. |
Guard for missing translucency value.
@hansemannn, @jquick-axway, @garymathews Updated the PR. |
Now that we have the Alloy PR up, I can quickly do the iOS part and do the docs there, adding the Android platform there as well. Not sure when QE can validate this PR, but the iOS one will be very straight-forward (this week'ish). |
FR Passed. Toolbar, its properties, methods & events from view seem to work as expected. You need to have custom theme with no actionbar in the app: builtin_themes.xml:
tiappp.xml:
Studio Ver: 4.9.1.201707200100 |
int width = widthDimension != null ? widthDimension.getAsPixels(toolbar):Toolbar.LayoutParams.WRAP_CONTENT; | ||
TiDimension heightDimension = source.getLayoutParams().optionHeight; | ||
int height = heightDimension != null ? heightDimension.getAsPixels(toolbar):Toolbar.LayoutParams.WRAP_CONTENT; | ||
res.setLayoutParams(new Toolbar.LayoutParams(width,height)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above, also create a 6.2.0
backport
*/ | ||
public void setLogo(Object object) { | ||
if (!TiApplication.isUIThread()) { | ||
TiMessenger.sendBlockingMainMessage(mainHandler.obtainMessage(TOOLBAR_SET_LOGO),object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after ,
there are other instances of this, please check
} | ||
|
||
/** | ||
* Handler for translucency change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor, fix spacing after *
here
@@ -21,9 +22,11 @@ | |||
import android.app.Activity; | |||
import android.content.Intent; | |||
import android.os.Message; | |||
import android.support.v7.app.AppCompatActivity;; | |||
import android.support.v7.app.AppCompatActivity; | |||
import org.appcelerator.titanium.view.TiUIView;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two ;;
at end of line
Remove unused import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: PASS
👍
Extract some properties as such. Add examples.
@garymathews, @jquick-axway, @hansemannn Update: and it seems to be working fine on Android. |
@ypbnv , @garymathews - I merged this by mistake before tidev/alloy#841. |
@lokeshchdhry It should not create issues, but won't be available for testing through Alloy before the changes from tidev/alloy#841 are applied. Classic Titanium will be fine. |
JIRA: https://jira.appcelerator.org/browse/TIMOB-17964
Needs to be merged first:
Description:
Having a toolbar as an ActionBar requires using a theme without a default one:
Very basic test case
Alloy test:
Documentation is on the way.
Files for the test case: