Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Added a screen annotation tool (experimental) #154

Merged
merged 6 commits into from
Jul 11, 2023
Merged

Conversation

SuhasDissa
Copy link
Member

@SuhasDissa SuhasDissa commented Jul 9, 2023

I just added a way to annotate screen while screen recording

DEMO:

VideoConvert_2023-07-09_23-45-57.mp4

@SuhasDissa SuhasDissa marked this pull request as ready for review July 11, 2023 10:57
@SuhasDissa
Copy link
Member Author

Hi there!

I'm excited to announce that I've finally made the annotation tool production ready. I've tested it thoroughly, but there may still be some bugs that I haven't found.

Would you mind taking a look at it and letting me know what you think? Any suggestions are welcome.

Thanks!

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

This looks really cool, thanks for your work so far!
I'll take a look as soon as possible!

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

This works really well! Most of my comments are only some minor formatting changes.

Please also make all the strings translatable by adding them to res/values/strings.xml and using the stringResource(stringId) function instead of hardcoding them in English.

Looks great otherwise!

@SuhasDissa
Copy link
Member Author

Please also make all the strings translatable by adding them to res/values/strings.xml and using the stringResource(stringId) function instead of hardcoding them in English.

I didn't want to mess with your strings file. I just added that strings for context. I suggest you to change those strings to more formal descriptions as you like.

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

Okay, makes sense!
Two other things we should do for user convenience before merging:

  1. Add a preference to disable the annotation tool, nevertheless the overlay should be enabled by default
  2. Prompt users to allow RecordYou to display above other apps when they start a screen recording (I think we can open the Android settings for that with an intent).

It doesn't matter to me who of us is going to quickly add these two things, as you prefer :)

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

Please use the Preferences object for handling with preferences instead of a Context#prefereces extension function, I'd like to keep that consistent across the whole app.
That'll be useful if we migrated to Jetpack Datastore for example.

@SuhasDissa

This comment was marked as off-topic.

@SuhasDissa
Copy link
Member Author

Please use the Preferences object for handling with preferences instead of a Context#prefereces extension function, I'd like to keep that consistent across the whole app. That'll be useful if we migrated to Jetpack Datastore for example.

Sorry for that. I thought you'd like that

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

I've seen something like that on other apps before already, and I do really like the approach.
However, in my opinion it should be used across the whole app then, and not only in a single file, which would make it very inconsistent and more difficult to maintain in general.

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

And our current approach should have a slightly better performance than using the extension functions because we don't recreated a SharedPreferences object for each preference, but that's mostly irrelevant anyways.

@SuhasDissa

This comment was marked as off-topic.

Fix minor formatting issues
Add annotation tool settings to bottom sheet

Switch back to using Preferences object for managing preferences
@Bnyro Bnyro merged commit 682ae24 into you-apps:main Jul 11, 2023
1 check passed
@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

the rememberPreference function lets you edit the variable directly and thus save the preference. so there's no need to save the preference each time the value change

Yes, that's pretty cool!
I wouldn't mind merging a PR for that if you'd like to work on it, but then all preferences should use it the same way.

@Bnyro
Copy link
Member

Bnyro commented Jul 11, 2023

And thanks for your contribution! <3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants