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

Custom text saved with pipe delimiter isnt loaded correctly #4534

Closed
monkeytypegeorge opened this issue Aug 13, 2023 · 10 comments · Fixed by #4619
Closed

Custom text saved with pipe delimiter isnt loaded correctly #4534

monkeytypegeorge opened this issue Aug 13, 2023 · 10 comments · Fixed by #4619
Assignees

Comments

@monkeytypegeorge
Copy link
Collaborator

No description provided.

@sanidhyas3s
Copy link
Contributor

More details about the issue and steps to replicate would be helpful. I would like to work on this.
@Miodec

@Miodec
Copy link
Member

Miodec commented Aug 16, 2023

If you enable pipe delimiter, then save the text this is|a test as custom text, when you load that text, it is incorrectly loaded as this|is|a|test

@sanidhyas3s
Copy link
Contributor

If you enable pipe delimiter, then save the text this is|a test as custom text, when you load that text, it is incorrectly loaded as this|is|a|test

Can't seem to find the settings corresponding to this, where can I find it under.

@Miodec
Copy link
Member

Miodec commented Aug 17, 2023

Test page > custom mode > change (this opens the custom text popup)

@sanidhyas3s
Copy link
Contributor

sanidhyas3s commented Aug 19, 2023

The way it works right now, is that when saving it breaks the text into 'space' separated words, and then joins them with the delimiter (| if pipe is enabled, space otherwise). Do we want to replace space with | at all? Because when saving | is considered as any other character.

Current Behavior:
"This is|a test" --save with or without pipe delimiter on--> "This", "is|a", "test" --load with delimiter--> "This|is|a|test"

Possible Fix Behavior
"This is|a test" --save with or without pipe delimiter on--> "This", "is|a", "test" --load with delimiter--> "This is|a test"

but it will create situations like this as well:
"This is a test" --save with or without pipe delimiter on--> "This", "is", "a", "test" --load with delimiter--> "This is a test"

So, if the expected correct behavior is simply keeping spaces, spaces and not replacing them with |, basically if you want to load a pipe-delimited text, you must also save a pipe-delimited text. If this is what is needed then this is just a simple fix, I will make a MR with that done. The fix will solve this particular problem but will takeaway "space being replaced with pipe automatically", so its a choice between the two, as there's no mid-way solution to this. @Miodec

This is more of a UX question at this point, I have never used the pipe-delimiter, so can't comment.

@flyingRa1jin
Copy link

@sanidhyas3s this looks pretty much like the expected behaviour. @Miodec Why would you want to use Pipe character when you are going to use it as a delimiter?

@Miodec
Copy link
Member

Miodec commented Sep 11, 2023

@Miodec Why would you want to use Pipe character when you are going to use it as a delimiter?

Dont understand the question sorry

@Miodec
Copy link
Member

Miodec commented Sep 11, 2023

The way it works right now, is that when saving it breaks the text into 'space' separated words, and then joins them with the delimiter (| if pipe is enabled, space otherwise). Do we want to replace space with | at all? Because when saving | is considered as any other character.

Current Behavior: "This is|a test" --save with or without pipe delimiter on--> "This", "is|a", "test" --load with delimiter--> "This|is|a|test"

Possible Fix Behavior "This is|a test" --save with or without pipe delimiter on--> "This", "is|a", "test" --load with delimiter--> "This is|a test"

but it will create situations like this as well: "This is a test" --save with or without pipe delimiter on--> "This", "is", "a", "test" --load with delimiter--> "This is a test"

So, if the expected correct behavior is simply keeping spaces, spaces and not replacing them with |, basically if you want to load a pipe-delimited text, you must also save a pipe-delimited text. If this is what is needed then this is just a simple fix, I will make a MR with that done. The fix will solve this particular problem but will takeaway "space being replaced with pipe automatically", so its a choice between the two, as there's no mid-way solution to this. @Miodec

This is more of a UX question at this point, I have never used the pipe-delimiter, so can't comment.

My bad looks like i missed this or forgot to respond. I think removing the automatic space to pipe conversion to correct the loading is a fair tradeoff.

@sanidhyas3s
Copy link
Contributor

@Miodec So, for a text saved without pipe delimiter wouldn't have pipe delimiters when loaded with pipe delimiter on, does that seem correct?

If yes, then simply we'll still break down the text based on space, and save it (text with pipe delimiter would just be saved as one long word) and then just combine them with space itself (this would be the only change).

sanidhyas3s added a commit to sanidhyas3s/monkeytype that referenced this issue Sep 11, 2023
Text saved with space separated words are now loaded with space only.

Fixes monkeytypegame#4534
@sanidhyas3s
Copy link
Contributor

I have made a PR with the fix as proposed in the previous message, you can test it, and merge it if the behavior matches the expected behavior.

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 a pull request may close this issue.

4 participants