Disallow duplicate thoughts #786

pushkov-fedor · 2020-07-10T14:16:52Z

Fixes #764

raineorshine

Thanks Fedor! It's working well for me. I'd like to loop in Bijay to ask if there's any way to avoid the direct DOM manipulation.

src/components/Editable.tsx

raineorshine · 2020-07-10T18:03:37Z

@shresthabijay I'd like your opinion since your familiar with this part of the code. Is there a better way to change the opacity of the Editable when a duplicate value is detected? I'm hesitant to use direct DOM manipulation. Is there a more idiomatic way to do this in React? Thanks!

shresthabijay · 2020-07-11T09:29:56Z

@shresthabijay I'd like your opinion since your familiar with this part of the code. Is there a better way to change the opacity of the Editable when a duplicate value is detected? I'm hesitant to use direct DOM manipulation. Is there a more idiomatic way to do this in React? Thanks!

I don't think there is any better solution to this. The problem is that we are restricted to not re-render Editable. I really haven't looked into why we cannot re-render Editable but I guess it has something to do with focus selection. I really think we should solve that issue instead of trying to find workaround to direct DOM manipulation.

raineorshine · 2020-07-11T14:47:13Z

@shresthabijay You're right, but that seems unavoidable. We can't re-render an Editable while the user is editing it, otherwise it would mess up their focus.

If only a style or className is changed, does that still re-render an element?

pushkov-fedor · 2020-07-16T12:54:06Z

@shresthabijay You're right, but that seems unavoidable. We can't re-render an Editable while the user is editing it, otherwise it would mess up their focus.

If only a style or className is changed, does that still re-render an element?

I think in our case we need to change appearance of element, which could be done by direct dom manipulation or via render function. If we use idiomatic "React" way, all changing in appearance happen after execution of render function, which one we can't use. Correct me please, if i'm wrong somewhere,

raineorshine · 2020-07-16T17:36:34Z

Yes, Bijay helped clarify that we should be able to use the render function without interfering with editing, but due to the current architecture it forces the DOM element to get re-rendered for some reason. That will need to get addressed in a separate issue.

For now, then, we can use direct DOM manipulation. The whole file deserves some refactoring in the future anyway.

raineorshine

There is a strange edge case that we need to address:

a
- ba
- bac|

Hit Backspace with the caret at the end of bac and then hit Enter. This will create a duplicate ba in the same context. The cause is that Enter incorrectly triggers splitThought because focusOffset is 2 and value.length is 3 since it has not been modified.

Try it yourself and give some thought to a proposed solution. How should we handle splitThought when the Editable is in a temporary duplicate state? Are there any other actions that depend on the caret position that may be affected as well?

pushkov-fedor · 2020-07-17T08:00:15Z

I think that we need to handle this situation in newThought action-creator. As i understand, correct result after hitting Enter is:

ba
bac

I suggest to use cursor and editingValue. Check if head value of cursor starts with editing value and length of head value is more than length of editingValue. After that check if we have duplicates in tail context of cursor. If these three checks return true then instead of dispatching splitThought we dispatch newThought.
I can add small function in newThought action-creator and invoke it at the end of
const split = !preventSplit && cursor && isFocusOnEditable && !showContexts && offset > 0 && offset < headValue(cursor).length

What do you think?

raineorshine · 2020-07-17T14:00:21Z

First of all, thank you for the clear explanation and proposal. That's exactly what I'm looking for.

I agree that this should happen in newThought where the splitThought condition is applied.

I think that we need to handle this situation in newThought action-creator. As i understand, correct result after hitting Enter is:

ba

bac

Yes, that's correct.

Check if head value of cursor starts with editing value and length of head value is more than length of editingValue.

It is incorrect to assume that the edit must come at the end of the thought. e.g. This would not the case of deleting the c from bca.

After that check if we have duplicates in tail context of cursor.

We shouldn't have to explicitly check for duplicates. It should only be a matter of the browser selection position.

What do you think?

I think there is an easier solution. According to state.thoughts, the cursor thought has not been edited yet. According to the DOM and browser selection it has. The issue occurs because both are being used as a source of truth. See if you can find a way to change the condition for splitChain by only looking at the editingValue.

Are there any other actions that depend on the caret position that may be affected as well?

This is important, too. You may have to search for the use of offset.

pushkov-fedor · 2020-07-20T11:38:16Z

As i understand, hitting 'Enter' on duplicate Thought should reset its value to the last not duplicate and trigger newThought action. In this way, the simplest approach to do it - we can add 'isDuplicate' attribute to editable div and retrieve its value in newThought action-creator. After checking this value we can decide either invoke splitThought or invoke newThought. But it's not 'React' way too.
I'm not sure that understand your way to determining what should be called splitThought or newThought just from browser selection and offset, because they don't tell us about duplicates

raineorshine · 2020-07-20T17:00:09Z

As i understand, hitting 'Enter' on duplicate Thought should reset its value to the last not duplicate and trigger newThought action.

Yes, although it should be noted that the "reset" is DOM-only; the state value was never actually changed.

In this way, the simplest approach to do it - we can add 'isDuplicate' attribute to editable div and retrieve its value in newThought action-creator. After checking this value we can decide either invoke splitThought or invoke newThought. But it's not 'React' way too.
I'm not sure that understand your way to determining what should be called splitThought or newThought just from browser selection and offset, because they don't tell us about duplicates

I still don't think it's the duplicate that matters here. That is a contingency. The general situation is that when the editingValue differs from the cursor value, then we should use the editingValue to detect if the thought should be split or not.

How about we each implement our preferred solution, and then compare the results?

pushkov-fedor · 2020-07-21T08:52:33Z

How about we each implement our preferred solution, and then compare the results?

I think it would be fun, let's try it :)

raineorshine · 2020-07-21T16:12:54Z

Okay, my solution is in ee1f252 ;)

pushkov-fedor · 2020-07-22T06:16:55Z

Looks like your won :)

raineorshine · 2020-07-22T23:25:59Z

Yours is quite clean! But yes, I think the generality and lack of additional DOM manipulation is preferred.

raineorshine · 2020-07-22T23:38:16Z

Just realized that merge-on-delete (inverse of split) can be used to create a duplicate:

Steps to Reproduce

ab
a
b

With the caret at the beginning (offset: 0) of b, hit Backspace.

Current Behavior

ab
ab

Expected Behavior

Maybe prevent the action completely and show an appropriate alert? What do you think?

pushkov-fedor · 2020-07-23T05:57:04Z

Maybe prevent the action completely and show an appropriate alert? What do you think?

Yes, i think we can handle this case in deleteEmptyThoughtOrOutdent shortcut and add another check.

raineorshine · 2020-07-23T21:03:58Z

Yes, let's do that.

raineorshine · 2020-07-24T14:19:05Z

There is a small regression:

Steps to Reproduce

a
[empty]

Try to delete the empty thought.

Current Behavior

Duplicate thoughts are not allowed within the same context

Expected Behavior

Empty thought should be deleted.

pushkov-fedor · 2020-07-27T04:11:47Z

There is a small regression:

Steps to Reproduce

a

[empty]

Try to delete the empty thought.

Current Behavior

Duplicate thoughts are not allowed within the same context

Expected Behavior

Empty thought should be deleted.

Fixed, thanks!

raineorshine · 2020-07-27T16:13:01Z

When Enter is hit on a duplicate thought, the "Duplicate thoughts are not allowed" alert is not closed.

raineorshine · 2020-07-27T16:15:08Z

Superscript regression:

Steps to Reproduce

a
- m
b
- mm
- m| [cursor]

Edit /b/m → b/mm

Current Behavior

Superscript position is not updated.

Expected Behavior

Superscript should be positioned correctly at the end of the thought text.

… null

…uplicateOnSplit check

…ught

…en thought is duplicate

… wrong render of new thought after editing and blur

… hits enter

raineorshine · 2020-09-02T15:40:58Z

src/action-creators/alert.ts

+  if (clearTimeout) {
+    // if clearAlertTimeoutId !== null, it means that previous alert hasn't been cleared yet. In this case cancel previous timeout and start new.
+    clearAlertTimeoutId && window.clearTimeout(clearAlertTimeoutId)
+    clearAlertTimeoutId = window.setTimeout(() => {
+      dispatch({
+        type: 'alert',
+        alertType,
+        showCloseLink,
+        value: null,
+      })
+      clearAlertTimeoutId = null
+    }, clearTimeout)
+  }


…ate-thoughts

raineorshine requested changes Jul 10, 2020

View reviewed changes

src/components/Editable.tsx Outdated Show resolved Hide resolved

src/components/Editable.tsx Outdated Show resolved Hide resolved

src/components/Editable.tsx Outdated Show resolved Hide resolved

pushkov-fedor force-pushed the disallow-duplicate-thoughts branch 2 times, most recently from 6d24338 to 4103e9d Compare July 11, 2020 07:14

raineorshine requested changes Jul 16, 2020

View reviewed changes

pushkov-fedor force-pushed the disallow-duplicate-thoughts branch 2 times, most recently from a6a0adb to e659e64 Compare July 24, 2020 08:48

pushkov-fedor force-pushed the disallow-duplicate-thoughts branch from e659e64 to 012dd73 Compare July 27, 2020 03:58

raineorshine force-pushed the dev branch from ff430c7 to da417a5 Compare July 28, 2020 01:21

pushkov-fedor force-pushed the disallow-duplicate-thoughts branch 2 times, most recently from d9feddb to 26a361c Compare July 30, 2020 10:55

Fedor Pushkov added 23 commits September 2, 2020 18:53

reset duplicateThoughtAlertTimeout after dispatching alert with value…

774e8d3

… null

Cancel dispatching duplicate alert after hitting Enter

30c8cfd

Rebase fix

b94663b

handle dispatching duplicate alert after hitting enter before unmount

aaa81cb

remove alert after 2 seconds in merge-on-delete duplicate case

543a410

add closure to avoid storing timeoutId in ref

e44d873

move closure out of component to prevent re-creation in render

f268ef7

prevent split if it creates duplicates

8816d77

stop multiple alert null invocation

18dd023

remove unnecessary offset and length check, which is overlaped by isD…

d0b819b

…uplicateOnSplit check

retrieve alert directly from store in order to get actual state

af414bc

wrap duplicate checking logic in function

076bd90

reset rendered thought value to non-duplicate on blur

c2255bc

dispatch alert and prevent creating empty thought on split

31d89d0

fix wrong position of superscript after append to the begining of tho…

5fbd386

…ught

fix wrong superscript position on blur

2dd26e0

dispatch alert and prevent creating new thought if user hits enter wh…

ffacb7d

…en thought is duplicate

fix wrong render of new thought after editing and blur

10e5cdf

flush thoughtChangeHandler at the beginning of onBlur in order to fix…

dc6a06a

… wrong render of new thought after editing and blur

dispatch alert null and clear timeout together if possible

964c0b6

always dispatch alert if current edited thought is duplicate and user…

f6a20ad

… hits enter

prevent new alert dispatching before alert null timer execution

fb88ea7

Add ability to create self-cleared alerts via action-creators/alert

3800131

pushkov-fedor force-pushed the disallow-duplicate-thoughts branch from 14219ea to a2461cc Compare September 2, 2020 11:53

reorganized self-cleared alert logic

4ee8c6a

pushkov-fedor force-pushed the disallow-duplicate-thoughts branch from a2461cc to 4ee8c6a Compare September 2, 2020 11:57

Merge branch 'dev' into disallow-duplicate-thoughts

4484bf8

raineorshine approved these changes Sep 2, 2020

View reviewed changes

raineorshine merged commit 9c2bf2c into cybersemics:dev Sep 2, 2020

anmolarora1 pushed a commit to anmolarora1/em that referenced this pull request Mar 10, 2021

Merge pull request cybersemics#786 from pushkov-fedor/disallow-duplic…

6f4ed66

…ate-thoughts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow duplicate thoughts #786

Disallow duplicate thoughts #786

pushkov-fedor commented Jul 10, 2020 •

edited

Loading

raineorshine left a comment

raineorshine commented Jul 10, 2020

shresthabijay commented Jul 11, 2020

raineorshine commented Jul 11, 2020

pushkov-fedor commented Jul 16, 2020

raineorshine commented Jul 16, 2020

raineorshine left a comment

pushkov-fedor commented Jul 17, 2020

raineorshine commented Jul 17, 2020

pushkov-fedor commented Jul 20, 2020

raineorshine commented Jul 20, 2020

pushkov-fedor commented Jul 21, 2020

raineorshine commented Jul 21, 2020

pushkov-fedor commented Jul 22, 2020

raineorshine commented Jul 22, 2020

raineorshine commented Jul 22, 2020

pushkov-fedor commented Jul 23, 2020 •

edited

Loading

raineorshine commented Jul 23, 2020

raineorshine commented Jul 24, 2020

pushkov-fedor commented Jul 27, 2020

Steps to Reproduce

Current Behavior

Expected Behavior

raineorshine commented Jul 27, 2020

raineorshine commented Jul 27, 2020

raineorshine Sep 2, 2020

Disallow duplicate thoughts #786

Disallow duplicate thoughts #786

Conversation

pushkov-fedor commented Jul 10, 2020 • edited Loading

raineorshine left a comment

Choose a reason for hiding this comment

raineorshine commented Jul 10, 2020

shresthabijay commented Jul 11, 2020

raineorshine commented Jul 11, 2020

pushkov-fedor commented Jul 16, 2020

raineorshine commented Jul 16, 2020

raineorshine left a comment

Choose a reason for hiding this comment

pushkov-fedor commented Jul 17, 2020

raineorshine commented Jul 17, 2020

pushkov-fedor commented Jul 20, 2020

raineorshine commented Jul 20, 2020

pushkov-fedor commented Jul 21, 2020

raineorshine commented Jul 21, 2020

pushkov-fedor commented Jul 22, 2020

raineorshine commented Jul 22, 2020

raineorshine commented Jul 22, 2020

Steps to Reproduce

Current Behavior

Expected Behavior

pushkov-fedor commented Jul 23, 2020 • edited Loading

raineorshine commented Jul 23, 2020

raineorshine commented Jul 24, 2020

Steps to Reproduce

Current Behavior

Expected Behavior

pushkov-fedor commented Jul 27, 2020

Steps to Reproduce

Current Behavior

Expected Behavior

raineorshine commented Jul 27, 2020

raineorshine commented Jul 27, 2020

Steps to Reproduce

Current Behavior

Expected Behavior

raineorshine Sep 2, 2020

Choose a reason for hiding this comment

pushkov-fedor commented Jul 10, 2020 •

edited

Loading

pushkov-fedor commented Jul 23, 2020 •

edited

Loading