[TIMOB-16436] Integrate appcompat themes to SDK #5348
Conversation
| } | ||
|
|
||
| ActionBarActivity activity = (ActionBarActivity) getWrappedActivity(); | ||
| actionBarProxy = new ActionBarProxy(activity); |
There was a problem hiding this comment.
Why are we removing the null check. Won't this create a new proxy everytime the method is called?
|
I've addressed comment and fix a blocker bug for 4.x+ devices. Also added a KitchenSink PR in ticket. |
|
FR failed. Otherwise stuff works fine though it looks slightly different from the base HOLO theme Device - Samsung S3 running 4.3 |
|
KS->Controls->ActionBar |
|
I have already tried to apply appcompat theme in Ti, and decided to go for actionbarsherlock because i had a lot of problem on android < 3.0 |
|
Menu Clicks are working now but there are still no graphics displayed on the Menu Items (Text Only). |
|
showAsAction is being ignored on 2X in Menu |
|
Fixed a few issues on 2.x actionbar. Also updated KS PR. |
|
@hieupham007 I remember actually not getting any actionbar in some cases. And if i remember correctly i had some crashes while changing orientation. But maybe the lib got updated since then. I tested it when it just got released. I will dig into my memory and get back to you ;) |
|
All looks good on this end. KS passed without any issues on the UI side.
Also left comments on the associated Kitchen Sink PR which need addressing |
| @@ -287,7 +287,7 @@ private TabProxy handleGetActiveTab() { | |||
| @Override | |||
| protected void handleOpen(KrollDict options) | |||
| { | |||
| Activity topActivity = TiApplication.getAppCurrentActivity(); | |||
| ActionBarActivity topActivity = (ActionBarActivity) TiApplication.getAppCurrentActivity(); | |||
There was a problem hiding this comment.
Do we need to cast this to an ActionBarActivity? We don't do anything specific with ActionBarActivity. Also not sure if TiApplication.getAppCurrentActivity(); may return some other type of activity or not.
There was a problem hiding this comment.
No, we don't need to cast it here, but since this entire class is using ActionBarActivity, not casting it would require an additional import of android.app.Activity, which would serve no purpose other than saving a cast.
There was a problem hiding this comment.
Well you would be risking a class cast exception if TiApplication.getAppCurrentActivity(); doesn't return an ActionBarActivity. It may be worth it to not cast instead of saving an import.
There was a problem hiding this comment.
I'll add an instanceof check here. If its not ActiionBarActivity, it will crash at a later time.
|
Code reviewed, left comments. |
|
Also noticed that there are unused imports in MenuItemProxy and TabGroupProxy, so you may want to clean that up. |
|
Addressed comments, also added support for more functionalities for 2.x ActionBar |
|
CR + FR ok. There is one discrepancy in the behavior of AppCompat library wrt Window.FEATURE_NO_TITLE In 4.0 and above it hides the actionBar. In 2.X it does not. Same issue reported here Since there is an obvious discrepancy in behavior and navbarHidden is not a native Android property, I am comfortable with dropping support for this property One caveat that we have not considered is the Android HIG. Specifically with our support for fullscreen. According to the docs here we should never show the action bar in a full screen window, but we have no code enforcing that behavior. setNavBarHidden(boolean) in TiBaseActivity needs to be refactored for ActionBar. Otherwise looks good. |
|
Code reviewed. Also ran anvil on this, and it has fewer errors than the master branch. There will be a separate ticket for removing the navbarHidden property. Request Accepted. |
[TIMOB-16436] Integrate appcompat themes to SDK
[TIMOB-16436] Integrate appcompat themes to SDK
Testing steps in JIRA.