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

recording: multiple fixes for better frame rate, padding, drawables #118

Merged
merged 15 commits into from
Apr 11, 2024

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Apr 8, 2024

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

Comment on lines +22 to +23
public var iconLeft: String? = null,
public var iconRight: String? = null,
Copy link
Member Author

Choose a reason for hiding this comment

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

they hold a base64 value similar to backgroundImage, if present in text fields, they hold a small icon at the start (before the text), or at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pauldambra we don't have that, shall we add it?

Rate and review has no icon

Copy link
Member

Choose a reason for hiding this comment

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

🤔 seems good to add to me... 👍

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I'm catching up here maybe... is it a custom library 🤔

I don't know how much we want to bake custom library stuff into the schema.

Willing to be led by you - I've no idea how popular this library is

Copy link
Member

Choose a reason for hiding this comment

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

Shall we split things and get the padding fix in and then follow up with this icon change if we decide to do it?

Copy link
Member Author

@marandaneto marandaneto Apr 10, 2024

Choose a reason for hiding this comment

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

Its not a custom library, the icon is custom, but the drawables on the 4 possible positions are part of the UI design system as well.
https://developer.android.com/reference/android/widget/TextView#getCompoundDrawables()

Copy link
Member

Choose a reason for hiding this comment

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

cool beans, I'll add this to schema/transformer 👍

will need to figure out what the defaults/rules are around display but that's just mechanical

Comment on lines 575 to 579
base64 = drawable.cloneAndToBase64(view)
// style.paddingTop = view.paddingTop.densityValue(displayMetrics.density)
// style.paddingBottom = view.paddingBottom.densityValue(displayMetrics.density)
// style.paddingLeft = view.paddingLeft.densityValue(displayMetrics.density)
// style.paddingRight = view.paddingRight.densityValue(displayMetrics.density)
Copy link
Member

Choose a reason for hiding this comment

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

need to keep the commented code?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but it is a TODO (have to figure out the offset), the padding fixes some cases and breaks others because of the drawable size (vectors), the OS adjusts to the right size but we don't have access to this info, so in some cases the padding will fix it, in some cases it will break it.

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

👍

@marandaneto marandaneto marked this pull request as ready for review April 10, 2024 15:05
@marandaneto marandaneto changed the title fix: recording img padding recording: multiple fixes for better frame rate, padding, drawables Apr 10, 2024
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

👍

@@ -31,6 +45,6 @@ internal class Debouncer(
}

companion object {
private const val ONE_FRAME_MS = 64L
private const val DELAY_MS = 500L
Copy link
Member

Choose a reason for hiding this comment

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

I can almost see us letting someone configure this... not-blocking here just seeding the idea in your head :)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I thought the same, lets see if this approach works better first, and happy to make it configurable otherwise.

Comment on lines +22 to +23
public var iconLeft: String? = null,
public var iconRight: String? = null,
Copy link
Member

Choose a reason for hiding this comment

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

cool beans, I'll add this to schema/transformer 👍

will need to figure out what the defaults/rules are around display but that's just mechanical

@marandaneto marandaneto merged commit be36b28 into main Apr 11, 2024
7 checks passed
@marandaneto marandaneto deleted the fix/img-padding branch April 11, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants