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.

Sign up for GitHub

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

Jump to bottom

Improve support for Sony TV devices #233

Closed
wants to merge 7 commits into from
Closed

Improve support for Sony TV devices #233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c81ffc
Improve extraction of current application identifier from dumpsys out…
martomo Feb 3, 2021
0abd708
Display friendly names for pre-installed applications on Sony TV.
martomo Feb 3, 2021
dc515b4
Support HDMI input detection for Sony TV devices.
martomo Feb 3, 2021
66840c2
Return hardware identifier for external input instead of HDMI port nu…
martomo Feb 4, 2021
7b67bd3
Use string replacement for hdmi_input test cases instead of splitting…
martomo Feb 6, 2021
38dab6f
Added hdmi_input test cases for non hardware definition and non TV In…
martomo Feb 6, 2021
5d1a394
Merge branch 'master' of https://github.com/JeffLIrion/python-android…
martomo Feb 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 17 additions & 6 deletions androidtv/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
INTENT_LAUNCH = "android.intent.category.LAUNCHER"
INTENT_HOME = "android.intent.category.HOME"

# Specify application identifier as CURRENT_APP from dumpsys output previously assigned to CURRENT_APP
VAR_CURRENT_APP_EXTRACTION = 'CURRENT_APP=${CURRENT_APP#*ActivityRecord{* * } CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && CURRENT_APP=${CURRENT_APP%\\}*}'
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't used.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, I guess it is used. Chrome on my phone only found it once on this page. So maybe it's OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is the core "logic" in regard to extracting the application identifier from the dumpsys output, either it being from dumpsys window windows or dumpsys activity a ..

It does the following operations:

  1. ${CURRENT_APP#*ActivityRecord{* * }
    Attempt to extract application activity from mResumedActivity or mFocusedApp
  2. ${CURRENT_APP#*{* * }
    Fallback to extract application activity from mCurrentFocus, in case mFocusedApp is not defined
  3. ${CURRENT_APP%%/*}
    Remove extraneous activity details for extracted application, leaving only identifier
  4. ${CURRENT_APP%\\}*}
    Delete trailing curly brace from identifier of extracted application, in case application has no activity


# echo '1' if the previous shell command was successful
CMD_SUCCESS1 = r" && echo -e '1\c'"
Expand All @@ -28,20 +30,29 @@
#: Determine whether the device is awake
CMD_AWAKE = "dumpsys power | grep mWakefulness | grep -q Awake"

#: Define current app
CMD_DEFINE_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && " + VAR_CURRENT_APP_EXTRACTION
Copy link
Owner

Choose a reason for hiding this comment

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

This constant seems unnecessary. Same goes for the other CMD_DEFINE_CURRENT_APP_* constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which way do you see this as potentially unnecessary?

Besides the main objective to deduplicate the code, as the majority of the complexity is already defined in VAR_CURRENT_APP_EXTRACTION.

The other use case I had in mind, using this module from a Python script and being able to use CURRENT_APP (directly) in a custom command using adb shell without the additional (CMD_CURRENT_APP) command to obtain the current_app.

Copy link
Owner

Choose a reason for hiding this comment

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

My comments from earlier were based on looking at this pull request on my phone, and the "Find in page" functionality wasn't working right. So I do see that these constants are used and that they make the code a bit more DRY.

My main concerns are that

  1. Adding more constants makes the pull request harder to review -- it's hard to see what is actually being changed.
  2. Aside from this pull request, it's easier to read the code if the full command is spelled out:
# I need to check what `VAR_CURRENT_APP_EXTRACTION` does
CMD_DEFINE_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && " + VAR_CURRENT_APP_EXTRACTION

# Here I know exactly what the ADB shell command is doing
CMD_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP"

The other use case I had in mind, using this module from a Python script and being able to use CURRENT_APP (directly) in a custom command using adb shell without the additional (CMD_CURRENT_APP) command to obtain the current_app.

The way I see it, anyone doing that is a power user. I think such a person would be more likely to look through these constants and piece together what they need in order to achieve their desired outcome OR simply formulate their ADB shell command from scratch rather than to pick out constants from this module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To assist with the review of the code, below is the diff of fully written commands by changed constant:

CMD_CURRENT_APP

-CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP
+CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && CURRENT_APP=${CURRENT_APP#*ActivityRecord{* * } CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && CURRENT_APP=${CURRENT_APP%\\}*} && echo $CURRENT_APP

CMD_CURRENT_APP_GOOGLE_TV

-CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP
+CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) && CURRENT_APP=${CURRENT_APP#*ActivityRecord{* * } CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && CURRENT_APP=${CURRENT_APP%\\}*} && echo $CURRENT_APP

CMD_HDMI_INPUT

-dumpsys activity starter | grep -o 'HDMIInputService\\/HW[0-9]' -m 1 | grep -o 'HW[0-9]'
+dumpsys activity starter | grep -E -o '(ExternalTv|HDMI)InputService/HW[0-9]' | grep -o 'HW[0-9]'

CMD_LAUNCH_APP

-CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + " --pct-syskeys 0 1; fi
+CURRENT_APP=$(dumpsys window windows | grep -E 'mCurrentFocus|mFocusedApp') && CURRENT_APP=${{CURRENT_APP#*ActivityRecord{{* * }} CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && CURRENT_APP=${{CURRENT_APP%\\}}*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + ' --pct-syskeys 0 1; fi

CMD_LAUNCH_APP_GOOGLE_TV

-CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${{CURRENT_APP%%/*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + " --pct-syskeys 0 1; fi
+CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) && CURRENT_APP=${{CURRENT_APP#*ActivityRecord{{* * }} CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && CURRENT_APP=${{CURRENT_APP%\\}}*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + ' --pct-syskeys 0 1; fi

Note: If you plan to compare these commands via android.adb_command service in Home Assistant, be sure to remove any escaped characters (e.g. \\, {{, }}) or to populate placeholders ({0}).

In regard to having more constants/variables, by simply looking at the above diffs you can see there is a lot of duplicate complexity, this should simplify any future changes to CURRENT_APP logic, as already mentioned in #233 (comment).


#: Get the current app
CMD_CURRENT_APP = "CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${CURRENT_APP#*{* * } && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP"
CMD_CURRENT_APP = CMD_DEFINE_CURRENT_APP + ' && echo $CURRENT_APP'

#: Define current app for a Google TV device
CMD_DEFINE_CURRENT_APP_GOOGLE_TV = 'CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) && ' + VAR_CURRENT_APP_EXTRACTION

#: Get the current app for a Google TV device
CMD_CURRENT_APP_GOOGLE_TV = "CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${CURRENT_APP%%/*} && echo $CURRENT_APP"
Copy link
Owner

Choose a reason for hiding this comment

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

It looks like you changed the command here (old first, new second):

CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) ...
CURRENT_APP=$(dumpsys activity a . | grep mResumedActivity) ...

Why the change? I don't own a Google TV device, so I can't test whether this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this particular change I looked at this Home Assistant Community topic (reference from #207). Indicating that both mCurrentFocus and mFocusedApp are no longer specified in the dumpsys window windows output.

Then I checked the alternative command (dumpsys activity a . | grep mResumedActivity) to obtain application activity details, which is also working on my Sony TV running Android 8.0.0, and noticed it has the same ActivityRecord entry as mFocusedApp.

mResumedActivity: ActivityRecord{35dbb2a u0 com.google.android.tvlauncher/.MainActivity t2719}

mFocusedApp=AppWindowToken{c791eb8 token=Token{a6bad1b ActivityRecord{35dbb2a u0 com.google.android.tvlauncher/.MainActivity t2719}}}

Rather than using the existing solution which relies on space (indentation) to extract the application activity details, I decided to change this approach to be more generic by looking for the ActivityRecord instead.

CMD_CURRENT_APP_GOOGLE_TV = CMD_DEFINE_CURRENT_APP_GOOGLE_TV + ' && echo $CURRENT_APP'

#: Get the HDMI input
CMD_HDMI_INPUT = "dumpsys activity starter | grep -o 'HDMIInputService\\/HW[0-9]' -m 1 | grep -o 'HW[0-9]'"
CMD_HDMI_INPUT = "dumpsys activity starter | grep -E -o '(ExternalTv|HDMI)InputService/HW[0-9]' | grep -o 'HW[0-9]'"

#: Launch an app if it is not already the current app
CMD_LAUNCH_APP = "CURRENT_APP=$(dumpsys window windows | grep mCurrentFocus) && CURRENT_APP=${{CURRENT_APP#*{{* * }} && CURRENT_APP=${{CURRENT_APP%%/*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + " --pct-syskeys 0 1; fi"
CMD_LAUNCH_APP_CONDITION = "if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + ' --pct-syskeys 0 1; fi'

#: Launch an app
CMD_LAUNCH_APP = CMD_DEFINE_CURRENT_APP.replace('{', '{{').replace('}', '}}') + ' && ' + CMD_LAUNCH_APP_CONDITION
Copy link
Owner

Choose a reason for hiding this comment

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

I know it's not DRY to copy & paste, but given that this module is simply a bunch of constants, I think it's acceptable here. Also, I think it's much more readable.

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 certainly agree in regard to readability, also slightly consider this to be a despicable solution, but it does escape all curly braces in the referenced constant to accompany the formatting done with this constant definition.

Although the complexity of extracting the CURRENT_APP variable from dumpsys output, from my experience, having multiple occurrences with duplicate logic, it's very easy to overlook certain occurrences when updating logic in regard to CURRENT_APP.

This is the main reason I took the DRY approach, by introducing the CMD_DEFINE_CURRENT_APP and VAR_CURRENT_APP_EXTRACTION constants, for reusability and to simplifying maintenance of CURRENT_APP logic.


#: Launch an app if it is not already the current app (for Google TV devices)
CMD_LAUNCH_APP_GOOGLE_TV = "CURRENT_APP=$(dumpsys activity a . | grep -E 'mResumedActivity' | cut -d ' ' -f 8) && CURRENT_APP=${{CURRENT_APP%%/*}} && if [ $CURRENT_APP != '{0}' ]; then monkey -p {0} -c " + INTENT_LAUNCH + " --pct-syskeys 0 1; fi"
#: Launch an app on a Google TV device
CMD_LAUNCH_APP_GOOGLE_TV = CMD_DEFINE_CURRENT_APP_GOOGLE_TV.replace('{', '{{').replace('}', '}}') + ' && ' + CMD_LAUNCH_APP_CONDITION

#: Get the state from ``dumpsys media_session``; this assumes that the variable ``CURRENT_APP`` has been defined
CMD_MEDIA_SESSION_STATE = "dumpsys media_session | grep -A 100 'Sessions Stack' | grep -A 100 $CURRENT_APP | grep -m 1 'state=PlaybackState {'"
Expand Down
4 changes: 2 additions & 2 deletions tests/test_androidtv_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@
GET_PROPERTIES_OUTPUT3D = GET_PROPERTIES_OUTPUT3.splitlines()[0]
GET_PROPERTIES_OUTPUT3E = '\n'.join(GET_PROPERTIES_OUTPUT3.splitlines()[:2])
GET_PROPERTIES_OUTPUT3F = '\n'.join(GET_PROPERTIES_OUTPUT3.splitlines()[:3])
GET_PROPERTIES_OUTPUT3G = '\n'.join(GET_PROPERTIES_OUTPUT3.splitlines()[:4]) + "HDMI"
GET_PROPERTIES_OUTPUT3G = '\n'.join(GET_PROPERTIES_OUTPUT3.splitlines()[:4]) + "HW2"

GET_PROPERTIES_DICT3A = {'screen_on': True,
'awake': False,
Expand Down Expand Up @@ -196,7 +196,7 @@
'is_volume_muted': None,
'volume': None,
'running_apps': None,
'hdmi_input': 'HDMI'}
'hdmi_input': 'HW2'}

GET_PROPERTIES_OUTPUT4 = """111Wake Locks: size=2
com.amazon.tv.launcher
Expand Down
2 changes: 1 addition & 1 deletion tests/test_basetv_async.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ async def test_get_hdmi_input(self):
"""Check that the ``get_hdmi_input`` function works correctly.

"""
with async_patchers.patch_shell("HDMI2")[self.PATCH_KEY]:
with async_patchers.patch_shell("HW2")[self.PATCH_KEY]:
with patch_calls(self.btv, self.btv._get_hdmi_input) as patched:
await self.btv.get_hdmi_input()
assert patched.called
Expand Down
33 changes: 27 additions & 6 deletions tests/test_basetv_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,23 @@
Device "eth0" does not exist.
"""

DEVICE_PROPERTIES_OUTPUT_SONY_TV = """Sony
BRAVIA 4K GB
SERIALNO
8.0.0
link/ether 11:22:33:44:55:66 brd ff:ff:ff:ff:ff:ff
link/ether ab:cd:ef:gh:ij:kl brd ff:ff:ff:ff:ff:ff
"""

DEVICE_PROPERTIES_DICT_SONY_TV = {
'manufacturer': 'Sony',
'model': 'BRAVIA 4K GB',
'serialno': 'SERIALNO',
'sw_version': '8.0.0',
'wifimac': '11:22:33:44:55:66',
'ethmac': 'ab:cd:ef:gh:ij:kl'
}

INSTALLED_APPS_OUTPUT_1 = """package:org.example.app
package:org.example.launcher
"""
Expand Down Expand Up @@ -350,6 +367,10 @@ def test_get_device_properties(self):
self.assertEqual(self.btv.device_properties["manufacturer"], "Google")
self.assertEqual(self.btv._cmd_get_properties_lazy_no_running_apps, constants.CMD_GOOGLE_TV_PROPERTIES_LAZY_NO_RUNNING_APPS)

with patchers.patch_shell(DEVICE_PROPERTIES_OUTPUT_SONY_TV)[self.PATCH_KEY]:
device_properties = self.btv.get_device_properties()
self.assertDictEqual(DEVICE_PROPERTIES_DICT_SONY_TV, device_properties)

def test_get_installed_apps(self):
""""Check that `get_installed_apps` works correctly.

Expand Down Expand Up @@ -493,14 +514,14 @@ def test_get_hdmi_input(self):
"""Check that the ``get_hdmi_input`` function works correctly.

"""
with patchers.patch_shell("HDMI2")[self.PATCH_KEY]:
self.assertEqual(self.btv.get_hdmi_input(), "HDMI2")
with patchers.patch_shell("HW2")[self.PATCH_KEY]:
self.assertEqual(self.btv.get_hdmi_input(), "HW2")

with patchers.patch_shell("HDMI2\n")[self.PATCH_KEY]:
self.assertEqual(self.btv.get_hdmi_input(), "HDMI2")
with patchers.patch_shell("HW2\n")[self.PATCH_KEY]:
self.assertEqual(self.btv.get_hdmi_input(), "HW2")

with patchers.patch_shell("HDMI2\r\n")[self.PATCH_KEY]:
self.assertEqual(self.btv.get_hdmi_input(), "HDMI2")
with patchers.patch_shell("HW2\r\n")[self.PATCH_KEY]:
self.assertEqual(self.btv.get_hdmi_input(), "HW2")

with patchers.patch_shell("")[self.PATCH_KEY]:
self.assertIsNone(self.btv.get_hdmi_input())
Expand Down