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

Fix/ime iob fix #834

Merged
merged 4 commits into from
Oct 16, 2019
Merged

Fix/ime iob fix #834

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
929eaf4
avoid calling CssStyleFormatter.mergeStyleAttributes if parentStyle o…
mzorz Jul 26, 2019
0ca13ce
added code to keep instance of InputConnection as long as parameters …
mzorz Jul 26, 2019
d20fb85
using WeakReference in InputConnection
mzorz Aug 12, 2019
2169aff
conditionally applying patch reusing InputConnection on Android Oreo …
mzorz Oct 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions aztec/src/main/kotlin/org/wordpress/aztec/AztecText.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ import android.view.MotionEvent
import android.view.View
import android.view.WindowManager
import android.view.inputmethod.BaseInputConnection
import android.view.inputmethod.EditorInfo
import android.view.inputmethod.InputConnection
import android.widget.CheckBox
import android.widget.EditText
import android.widget.Toast
Expand All @@ -70,6 +72,7 @@ import org.wordpress.aztec.handlers.ListHandler
import org.wordpress.aztec.handlers.ListItemHandler
import org.wordpress.aztec.handlers.PreformatHandler
import org.wordpress.aztec.handlers.QuoteHandler
import org.wordpress.aztec.ime.EditorInfoUtils
import org.wordpress.aztec.plugins.IAztecPlugin
import org.wordpress.aztec.plugins.IToolbarButton
import org.wordpress.aztec.source.Format
Expand Down Expand Up @@ -291,6 +294,9 @@ open class AztecText : AppCompatEditText, TextWatcher, UnknownHtmlSpan.OnUnknown

private var focusOnVisible = true

var inputConnection: InputConnection? = null
var inputConnectionEditorInfo: EditorInfo? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should keep a WeakReference to those objects. These are objects used deeply withing the Android editing subsystem and I'm not sure if we'll introduce other bugs if we keep strong references to them. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, updated the inputConnection to use a WeakReference in d20fb85.

The var inputConnectionEditorInfo is an actual new instance that copies the attributes values by value on this line so, left as it was 👍


interface OnSelectionChangedListener {
fun onSelectionChanged(selStart: Int, selEnd: Int)
}
Expand Down Expand Up @@ -660,6 +666,36 @@ open class AztecText : AppCompatEditText, TextWatcher, UnknownHtmlSpan.OnUnknown
}
}

override fun onCreateInputConnection(outAttrs: EditorInfo) : InputConnection {
// initialize inputConnectionEditorInfo
if (inputConnectionEditorInfo == null) {
inputConnectionEditorInfo = outAttrs
}

// now init the InputConnection, or replace if EditorInfo contains anything different
if (inputConnection == null || !EditorInfoUtils.areEditorInfosTheSame(outAttrs, inputConnectionEditorInfo!!)) {
// we have a new InputConnection to create, save the new EditorInfo data and create it
// we make a copy of the parameters being received, because super.onCreateInputConnection may make changes
// to EditorInfo params being sent to it, and we want to preserve the same data we received in order
// to compare.
// (see https://android.googlesource.com/platform/frameworks/base/+/jb-mr0-release/core/java/android/widget/
// TextView.java#5404)
inputConnectionEditorInfo = EditorInfoUtils.copyEditorInfo(outAttrs)
val localInputConnection = super.onCreateInputConnection(outAttrs)
if (localInputConnection == null) {
// in case super returns null, let's just observe the base implementation, no need to make
// an InputConnectionWrapper of a null target
return localInputConnection
}
// if non null, wrap the new InputConnection around our wrapper
mkevins marked this conversation as resolved.
Show resolved Hide resolved
//inputConnection = AztecTextInputConnectionWrapper(localInputConnection, this)
inputConnection = localInputConnection
}

// returnn the existing inputConnection
return inputConnection!!
}

override fun onRestoreInstanceState(state: Parcelable?) {
disableTextChangedListener()

Expand Down
55 changes: 55 additions & 0 deletions aztec/src/main/kotlin/org/wordpress/aztec/ime/EditorInfoUtils.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package org.wordpress.aztec.ime

import android.os.Build
import android.view.inputmethod.EditorInfo
import java.util.Arrays

object EditorInfoUtils {
@JvmStatic
fun areEditorInfosTheSame(ed1: EditorInfo, ed2: EditorInfo): Boolean {
if (ed1 == ed2) {
mkevins marked this conversation as resolved.
Show resolved Hide resolved
return true
}

if (ed1.actionId == ed2.actionId
Copy link
Contributor

Choose a reason for hiding this comment

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

np: Perhaps some comments here can be helpful to provide context on this comparison

&& (ed1.actionLabel != null && ed1.actionLabel.equals(ed2.actionLabel) || ed1.actionLabel == null && ed2.actionLabel == null)
&& ed1.inputType == ed2.inputType
&& ed1.imeOptions == ed2.imeOptions
&& (ed1.privateImeOptions != null && ed1.privateImeOptions.equals(ed2.privateImeOptions) || ed1.privateImeOptions == null && ed2.privateImeOptions == null)
&& ed1.initialSelStart == ed2.initialSelStart
&& ed1.initialSelEnd == ed2.initialSelEnd
&& ed1.initialCapsMode == ed2.initialCapsMode
&& ed1.fieldId == ed2.fieldId
) {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N_MR1) {
// specific comparisons here
if (ed1.contentMimeTypes != null && ed2.contentMimeTypes != null) {
return Arrays.equals(ed1.contentMimeTypes, ed2.contentMimeTypes)
}
}
return true
}
return false
}

@JvmStatic
fun copyEditorInfo(ed1: EditorInfo) : EditorInfo {
val copy = EditorInfo()
copy.actionId = ed1.actionId
copy.actionLabel = ed1.actionLabel?.toString()
copy.inputType = ed1.inputType
copy.imeOptions = ed1.imeOptions
copy.privateImeOptions = ed1.privateImeOptions?.toString()
copy.initialSelStart = ed1.initialSelStart
copy.initialSelEnd = ed1.initialSelEnd
copy.initialCapsMode = ed1.initialCapsMode
copy.fieldId = ed1.fieldId
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N_MR1) {
// specific comparisons here
if (ed1.contentMimeTypes != null) {
copy.contentMimeTypes = Arrays.copyOf(ed1.contentMimeTypes, ed1.contentMimeTypes.size)
}
}
return copy
}
}
4 changes: 3 additions & 1 deletion aztec/src/main/kotlin/org/wordpress/aztec/plugins/CssUnderlinePlugin.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ class CssUnderlinePlugin : ISpanPostprocessor, ISpanPreprocessor {
if (hiddenSpan.TAG == SPAN_TAG) {
val parentStyle = hiddenSpan.attributes.getValue(CssStyleFormatter.STYLE_ATTRIBUTE)
val childStyle = calypsoUnderlineSpan.attributes.getValue(CssStyleFormatter.STYLE_ATTRIBUTE)
hiddenSpan.attributes.setValue(CssStyleFormatter.STYLE_ATTRIBUTE, CssStyleFormatter.mergeStyleAttributes(parentStyle, childStyle))
if (parentStyle != null && childStyle != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this part of the InputConnection issue, or solely part of #832 and just bundled with this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh that's correct it's part of #832 but I needed to keep it within this patch so my monkey runs wouldn't be capped by finding the issue fixed in #832 first and then stopping

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the PR has been reverted, the guard here also got reverted. So, #831 got opened again but I think it makes sense to re-address it on its own now. Can you wrangle that @mkevins ?

Copy link
Contributor

@mkevins mkevins Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My understanding is that this particular commit was included in both #832, and #834, and was accidentally reverted in the revert of #834, so we need to revert the revert. Does that match your understanding @hypest ? I've created this branch here: https://github.com/wordpress-mobile/AztecEditor-Android/compare/issue/831-css-underline-illegal-state-revert-revert, but so far I've not been successful in reproducing the issue on develop using the monkeys (adb -s emulator-5554 shell monkey -p org.wordpress.aztec -v 1000000). Do we have any documented steps to manually reproduce the crash?

Copy link
Contributor

Choose a reason for hiding this comment

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

so we need to revert the revert. Does that match your understanding @hypest ?

Heh, I wouldn't just call it "revert the revert" anymore as it's not clear if it would bring back other things besides the guard but, judging from the branch you shared I'd say that yeah, re-instating the guard is the goal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any documented steps to manually reproduce the crash?

From what I recall, there weren't steps to reliably reproduce the crash, not sure if @mzorz has a better recollection on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👋 ok been looking into this, this is how it goes: the reverted code made this issue reappear #831, and #832 has the fix we need to re-apply so, basically adding that check back will prevent the crash from happening (that means it is my understanding @mkevins 's branch has the correct code to re-apply the condition check 👍 ).
Also back then, I wasn't able to reproduce manually, but only when running the monkeys.
I'd say let's create a PR from @mkevins branch and let's apply it. Thanks for looking into this @hypest and @mkevins

hiddenSpan.attributes.setValue(CssStyleFormatter.STYLE_ATTRIBUTE, CssStyleFormatter.mergeStyleAttributes(parentStyle, childStyle))
}

// remove the extra child span
spannable.removeSpan(calypsoUnderlineSpan)
Expand Down