[Android] Table element support #5860

golddove · 2021-05-21T17:25:29Z

Related Issue

Fixes #5725

Description

Fixed object model to use the appropriate VerticalContentAlignment enum
Fixed object model default border color values to remove excess hex characters
Added Table, TableCell renderers implemented in Kotlin, and TableCellRenderArgs : RenderArgs for passing context to cell renderer
[DEPRECATION] ContainerRenderer.ApplyPadding was doing two things, so it is now separated into applyPadding and applyContainerStyle
[TODO] Awaiting host config support for header text style (using heading style instead, for now)

Sample Card

v1.5\Elements\Table
Added v1.5\Tests\ColumnSetTableComparison

How Verified

Manual verification

Microsoft Reviewers: Open in CodeFlow

samples/v1.5/Test/TableColumnSetComparison.json

source/android/adaptivecards/build.gradle

.../android/adaptivecards/src/main/java/io/adaptivecards/renderer/readonly/TableCellRenderer.kt

almedina-ms · 2021-05-26T17:28:09Z

source/android/adaptivecards/src/main/java/io/adaptivecards/renderer/RenderArgs.java

+     */
+    public HorizontalAlignment getHorizontalAlignment()
+    {
+        return m_horizontalAlignment;


why is horizontal alignment passed down as part of the RenderArgs, do table or other containers pass down the property to their child elements?

You can set horizontalCellContentAlignment on table and table row. Per the spec, this property changes the horizontal alignment of the cell's elements (specifically, TextBlock, Image, and RichTextBlock, which all have a horizontalAlignment property)

are other Container elements going to have the same property at some point of the future or is this only planned for table cells?

almedina-ms · 2021-05-26T17:34:16Z

source/android/adaptivecards/build.gradle

    androidTestImplementation('androidx.test.espresso:espresso-core:3.1.0', {
        exclude group: 'com.android.support', module: 'support-annotations'
    })
    implementation 'androidx.appcompat:appcompat:1.0.0'
    implementation 'androidx.constraintlayout:constraintlayout:1.1.3'
    implementation 'com.google.android:flexbox:1.0.0'
    testImplementation 'junit:junit:4.12'
+    implementation "androidx.core:core-ktx:1.5.0-rc02"


is the use of Kotlin required/necessary for any piece of implementation or is it only used because it's easier to implement with it?

No, not required. As our partners use Kotlin, we can start to leverage features to make a better API (e.g. marking some fields as internal instead of making everything public, marking required parameters for null safety, etc.). So, for the two new classes here, I've used Kotlin to kickstart that migration.

in the case of APIs, are java and kotlin interchangeable? would there be any breaking changes if we fully migrate to kotlin?

almedina-ms · 2021-05-26T18:05:25Z

source/shared/cpp/ObjectModel/CollectionTypeElement.cpp

@@ -6,7 +6,9 @@

 using namespace AdaptiveCards;

-CollectionTypeElement::CollectionTypeElement(CardElementType type, ContainerStyle style, VerticalContentAlignment alignment) :
+CollectionTypeElement::CollectionTypeElement(CardElementType type,


are the object model changes in their own PR? I feel like reviewing those changes by themselves could be better

Yes, they are in their own PR (created separately). Once that is merged, this PR should stop showing those in the diff.

almedina-ms · 2021-05-26T18:06:24Z

source/android/build.gradle

@@ -1,25 +0,0 @@
-// Top-level build file where you can add configuration options common to all sub-projects/modules.


why is this file being removed?

Thanks, I was playing around with .kts version, but decided against it. This didn't get re-added because of gitignore.

almedina-ms · 2021-05-26T18:06:36Z

source/android/settings.gradle

@@ -1 +0,0 @@
-include ':mobilechatapp', ':mobile', ':adaptivecards'


same with this one

almedina-ms · 2021-05-26T19:17:33Z

.../android/adaptivecards/src/main/java/io/adaptivecards/renderer/readonly/TableCellRenderer.kt

+            layoutParams = TableRow.LayoutParams(0, TableLayout.LayoutParams.MATCH_PARENT)
+        }
+
+        cellLayout.updateLayoutParams<ViewGroup.MarginLayoutParams> {


if you could move this part to a new method it could be really good for readability, this render method is almost 100 lines long

almedina-ms · 2021-05-26T19:18:14Z

source/android/adaptivecards/src/main/java/io/adaptivecards/renderer/readonly/RendererUtil.java

@@ -156,4 +157,17 @@ public void handleTag(boolean opening, String tag, Editable output, XMLReader xm
            }
        }
    }
+
+    static HorizontalAlignment computeHorizontalAlignment(RenderArgs renderArgs, HorizontalAlignment declaredAlignment)


getHorizontalAlignment

Same comment as above. get/set should be reserved for "property"-like accessors/setters

almedina-ms · 2021-05-26T19:20:06Z

.../android/adaptivecards/src/main/java/io/adaptivecards/renderer/readonly/FactSetRenderer.java

@@ -46,14 +46,14 @@ public static FactSetRenderer getInstance()
        return s_instance;
    }

-    static TextView createTextView(Context context, CharSequence text, FactSetTextConfig textConfig, HostConfig hostConfig, long spacing, ContainerStyle containerStyle)
+    static TextView createTextView(Context context, RenderArgs renderArgs, CharSequence text, FactSetTextConfig textConfig, HostConfig hostConfig, long spacing, ContainerStyle containerStyle)


same as previously, I think moving RenderArgs as the last parameter may make more sense consistency wise

almedina-ms · 2021-05-26T19:20:34Z

...ndroid/adaptivecards/src/main/java/io/adaptivecards/renderer/readonly/ContainerRenderer.java

+     * @param inherited the ContainerStyle inherited through RenderArgs provided by parent
+     * @return the ContainerStyle to apply
+     */
+    public static ContainerStyle computeContainerStyle(ContainerStyle declared, ContainerStyle inherited)


getContainerStyle

almedina-ms · 2021-05-26T19:21:56Z

...ndroid/adaptivecards/src/main/java/io/adaptivecards/renderer/readonly/ContainerRenderer.java

+        applyPadding(computedContainerStyle, parentContainerStyle, collectionElementView, hostConfig, false);
+    }
+
+    public static void applyPadding(ContainerStyle computedContainerStyle, ContainerStyle parentContainerStyle, ViewGroup collectionElementView, HostConfig hostConfig, boolean hasBorder)


we may have to figure out at some point if we want static methods to start with a lower case or an upper case letter

Java/Kotlin convention is to start with lowercase

I think I got confused because of the deprecated method above

* Kotlin support * Table renderer * Fix style inheriting * Re-SWIG after merge * [Shared] Optional HorizontalAlignment, VerticalContentAlignment * Re-swig * PR feedback * Remove file * Remove file * Update Android tests * Fix iOS build * value_or for iOS * Restore .gradle files * Move RenderArgs to the end * PR feedback refactoring, move to stable kotlin lib version

golddove added 4 commits May 18, 2021 18:54

Kotlin support

81a5a81

Table renderer

78cc56d

Merge branch 'main' into golddove/5725

1d655ad

Fix style inheriting

f3f8c89

golddove requested review from jwoo-msft, RebeccaAnne, paulcam206 and almedina-ms May 21, 2021 17:25

Merge branch 'main' into golddove/5725

1a9cf13

paulcam206 requested changes May 21, 2021

View reviewed changes

ghost added the Needs: Author Feedback label May 21, 2021

golddove added 7 commits May 24, 2021 19:59

Merge branch 'main' into golddove/5725

570f8c2

Re-SWIG after merge

6c64e58

[Shared] Optional HorizontalAlignment, VerticalContentAlignment

b9db465

Merge branch 'golddove/table-shared-fix' into golddove/5725

91b6397

Re-swig

f3b2da4

Merge branch 'golddove/table-shared-fix' into golddove/5725

4cf476b

PR feedback

09d27a7

ghost removed the Needs: Author Feedback label May 26, 2021

golddove requested a review from paulcam206 May 26, 2021 15:27

golddove added 7 commits May 26, 2021 11:32

Remove file

ef21716

Remove file

9a481ef

Update Android tests

e5cb674

Fix iOS build

4ba7a86

Merge branch 'golddove/table-shared-fix' into golddove/5725

ac3473f

value_or for iOS

d115a69

Merge 'golddove/table-shared-fix' into golddove/5725

e7281b9

almedina-ms requested changes May 26, 2021

View reviewed changes

ghost added the Needs: Author Feedback label May 26, 2021

Merge branch 'main' into golddove/5725

389b9b2

ghost removed the Needs: Author Feedback label May 27, 2021

golddove added 2 commits May 27, 2021 13:05

Restore .gradle files

3ba6f59

Move RenderArgs to the end

3352b33

golddove requested a review from almedina-ms May 27, 2021 17:15

paulcam206 approved these changes May 27, 2021

View reviewed changes

almedina-ms approved these changes May 27, 2021

View reviewed changes

PR feedback refactoring, move to stable kotlin lib version

be2e4b1

almedina-ms approved these changes May 27, 2021

View reviewed changes

Merge branch 'main' into golddove/5725

1412b6b

golddove enabled auto-merge (squash) May 27, 2021 23:09

golddove merged commit d7fd2d4 into main May 27, 2021

golddove deleted the golddove/5725 branch May 27, 2021 23:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Table element support #5860

[Android] Table element support #5860

golddove commented May 21, 2021 •

edited by ghost

almedina-ms May 26, 2021

golddove May 26, 2021

almedina-ms May 27, 2021

almedina-ms May 26, 2021

golddove May 26, 2021

almedina-ms May 27, 2021

almedina-ms May 26, 2021

golddove May 26, 2021

almedina-ms May 26, 2021

golddove May 27, 2021

almedina-ms May 26, 2021

almedina-ms May 26, 2021

almedina-ms May 26, 2021

golddove May 26, 2021

almedina-ms May 26, 2021

almedina-ms May 26, 2021

almedina-ms May 26, 2021

golddove May 26, 2021

almedina-ms May 27, 2021

		@@ -1,25 +0,0 @@
		// Top-level build file where you can add configuration options common to all sub-projects/modules.

		@@ -1 +0,0 @@
		include ':mobilechatapp', ':mobile', ':adaptivecards'

[Android] Table element support #5860

[Android] Table element support #5860

Conversation

golddove commented May 21, 2021 • edited by ghost

Related Issue

Description

Sample Card

How Verified

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

golddove commented May 21, 2021 •

edited by ghost