'did-change-text' event is not fired on empty("") to empty("") change after PR#274

[t9md]

@maxbrunsfeld

After PR #274, empty("") to empty("") text change no longer fire did-change-text.
This seems to be a bit confusing since non empty same text change still fire did-change-text(e.g. "abc" to "abc").
And noticed this change breaks at least select-list's test-spec. Does this change intentional?

Impact

Some pkg feature assuming editor.setText("") always fire editor.onDidChange event, but this is no longer true after #274.

• assuming selectListView.reset() always fire event

• But not fire when compactedChanges length become 0 in"" to "" change

At least I noticed select-list's spec was broken by this change here.

Reproduce

1. Running following code in chrome-dev console.
2. Result is different between v1.23.0 and later.
   • Atom-v1.23.0, change event fired for both "abc" to "abc" and empty "" to empty "".
   • later version: change event fired for "abc" to "abc" but not fired for empty "" to empty "".

async function test() {
  const editor = await atom.workspace.open("")

  console.log('abc to abc change')
  {
    editor.setText("abc")
    const disposable = editor.onDidChange(change => {
      disposable.dispose()
      console.log("Fired", change);
    })
    editor.setText("abc")
  }

  console.log('empty to empty change')
  {
    editor.setText("")
    const disposable = editor.onDidChange(change => {
      disposable.dispose()
      console.log("Fired", change);
    })
    editor.setText("")
  }
}

test()

[maxbrunsfeld]

I think ideally, for performance reasons, we would never invoke onDidChangeText listeners unless the text actually changed. But in your abc example, we would have to do an extra string comparison to detect a no-op change, so it is probably not worth it.

So I guess I am happy with the current behavior. How bad is the breakage in the atom-select-list specs?

[t9md]

Not sure other breakage are exist. Found command-palette use SelectListView.prototype.reset here. IMO, its better to fire did-change-text regardless of old and new text.

• For compatibility to older behavior. Checking all the usage, unnoticed breakage is difficult.
• I feel setting empty text("") like in selectListView.reset() is common pattern and it's easy to overlook to notice this caveat(only empty-to-empty case is not fired).

https://github.com/atom/command-palette/blob/07610fc5819a3648b26b795873bc008dbe7436a0/lib/command-palette-view.js#L118

[maxbrunsfeld]

Can you explain a case where a package relies on onDidChangeText listeners getting called even if the text did not change? I'm not quite understanding how your example about reset necessarily relates to onDidChangeText.

[t9md]

At least these core pkg calling selectListView.reset not sure impact of each.

• selectListView.

$ ag selectListView.reset
git-diff/lib/diff-list-view.js
44:    this.selectListView.reset()

snippets/lib/snippets-available.js
51:      this.selectListView.reset()

encoding-selector/lib/encoding-list-view.js
61:    this.selectListView.reset()

grammar-selector/lib/grammar-list-view.js
59:    this.selectListView.reset()

command-palette/lib/command-palette-view.js
111:      this.selectListView.reset()

fuzzy-finder/lib/fuzzy-finder-view.js
168:      this.selectListView.reset()

symbols-view/lib/symbols-view.js
188:    this.selectListView.reset();

Can you explain a case where a package relies on onDidChangeText listeners getting called even if the text did not change? I'm not quite understanding how your example about reset necessarily relates to onDidChangeText.

1. selectListView.reset set empty text "" to queryEditor.

https://github.com/atom/atom-select-list/blob/ee325e75833acc84af9f9c90aed30157e1816f36/src/select-list-view.js#L64-L66

2. It fire queryEditor.onDidChange event, result in call to didChangeQuery

https://github.com/atom/atom-select-list/blob/ee325e75833acc84af9f9c90aed30157e1816f36/src/select-list-view.js#L27

3. didChangeQuery eventually call prop.didChangeQuery (if set by selectListView's client pkg).

https://github.com/atom/atom-select-list/blob/ee325e75833acc84af9f9c90aed30157e1816f36/src/select-list-view.js#L264

[t9md]

Why I cannot answer how bad this change is prop.didChangeQuery is set client pkg and impact is vary from client pkg. So to make selectLisView compatible with old behavior, following workaround can be possible.

• Old

  reset () {
  this.refs.queryEditor.setText('')
  }

• New

  reset () {
  if (this.refs.queryEditor.getText === "") {
  this.didChangeQuery()
  } else {
  this.refs.queryEditor.setText('')
  }
  }

[maxbrunsfeld]

I still don't understand why we should call didChangeQuery if the query did not change. What is the calling code that depends on didChangeQuery being called even if the query was and remains empty?

[t9md]

This is body of didChangQuery(), it refresh rendered items by calling this.computeItems(). And if reset is called launch time, it is expecting render fresh item by reset()ting., but if reset() no longer re-render items, it fail to gather newly added command in command-palette case.

I've not investigated well yet, so above explanation is just a though I simulated in my mind.

  didChangeQuery () {
    if (this.props.didChangeQuery) {
      this.props.didChangeQuery(this.getFilterQuery())
    }

    this.computeItems()
  }

[t9md]

I too not clearly understand how is actual bad impact by not calling onDidChange with empty to empty text. What I wanted to report is it introduce behavioral change in selectListView.reset and investigating each impact like we should add workaround code or not is not what I wanted to do. So asked if it's intentional.

[maxbrunsfeld]

Ok, I think I see what you're saying. Let me try to summarize. In atom-select-list, SelectList.reset is probably expected by packages (e.g. command-palette) to always recompute the items. Before, this would automatically happened via a call to SelectList.didChangeQuery. Now, it does not happen if the query was already blank.

I think it does make sense to change atom-select-list in order to fix this. I would make a slight tweak to your New code:

reset () {
  if (this.refs.queryEditor.getText === "") {
    this.computeItems()
  } else {
    this.refs.queryEditor.setText('')
  }
}

This way, we'll recompute the items but avoid calling the didChangeQuery hook, because the query has not actually changed. Does that make sense?

[maxbrunsfeld]

Thanks for reporting this and determining the cause of the problem. I agree that if there's user-facing impact we need to fix it. It just took me a minute to understand the problem.

[t9md]

@maxbrunsfeld Your modification on reset workaround code does make sense. My original code is just trying to stupidly compatible with older behavior for less chance of breakage. Your summarization is also correct. Thank you for understanding.