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

Revert clipboard to use json #582

Merged
merged 3 commits into from
Jun 22, 2019
Merged

Conversation

comodoro
Copy link
Contributor

Finally resolve #357 the least inventive way. Reasons in the issue, unable to fix toml. By the way if more exotic languages where Unicode is needed for dictation are supported, this will become an issue again with aliases, macros etc.

@LexiconCode LexiconCode added Caster Issues pertaining to primarily the Caster project. Complete Pull request is complete and tested as defined by Contributor High Priority labels Jun 16, 2019
@kendonB
Copy link
Collaborator

kendonB commented Jun 16, 2019

#485 still works after this change, FYI

@kendonB
Copy link
Collaborator

kendonB commented Jun 16, 2019

Saw this error just now:

<type 'exceptions.ValueError'>
Extra data: line 1 column 4 - line 4 column 1 (char 3 - 56)
<traceback object at 0x12E19FA8>
  File "C:\Users\kmbel\Documents\caster\castervoice\lib\utilities.py", line 137, in load_json_file
    result = json.load(json_file)

  File "C:\Python27\Lib\json\__init__.py", line 291, in load
    **kw)

  File "C:\Python27\Lib\json\__init__.py", line 339, in loads
    return _default_decoder.decode(s)

  File "C:\Python27\Lib\json\decoder.py", line 367, in decode
    raise ValueError(errmsg("Extra data", s, end, len(s)))

@comodoro
Copy link
Contributor Author

@kendonB Could you please specify more closely when does the error occur and contents of clipboard.json?

@kendonB
Copy link
Collaborator

kendonB commented Jun 17, 2019

I see it upon startup and I don't see clipboard.json in there at all. Only toml. Do you need to send it to .caster if it's not already there?

@comodoro
Copy link
Contributor Author

Oh, (facepalm), I forgot that everyone will still have clipboard.toml in their settings.toml. If you change SAVED_CLIPBOARD_PATH to clipboard.json, it should work.

@kendonB
Copy link
Collaborator

kendonB commented Jun 17, 2019

Shouldn't the PR try to do that itself?

@comodoro
Copy link
Contributor Author

It does with settings.py, however it cannot touch your already generated user settings. It would be possible to create new constant in settings.py for this, but the old one would still hang in users' files, so I did not (it is the same as with moving to toml before)

@comodoro
Copy link
Contributor Author

Although after some thought I can see its merit in reducing users' disturbance. What do you think?

@kendonB
Copy link
Collaborator

kendonB commented Jun 17, 2019

I think it's fine to change the path in settings.toml via settings.py if it's USER_DIR + "data/clipboard.toml"

@comodoro
Copy link
Contributor Author

@kendonB Do you mean like adding this specific check (probably in the _init function)? It looks a bit suspicious, but with clear indication that it is temporary I have no problem with that.

@kendonB
Copy link
Collaborator

kendonB commented Jun 17, 2019

I was thinking it would be in settings.py? Because that already writes new settings elements into settings.toml and this would just instead write over an existing settings element.

kendonB added a commit to kendonB/Caster that referenced this pull request Jun 17, 2019
@LexiconCode LexiconCode removed the Complete Pull request is complete and tested as defined by Contributor label Jun 18, 2019
@comodoro
Copy link
Contributor Author

@kendonB Sorry for the delay. Is the migrate commit what you had in mind? It is ugly and duplicates I/O (because of impirting order; utilities have to be refactored), but I am hoping we will be able to remove it before merging into master.

@kendonB
Copy link
Collaborator

kendonB commented Jun 21, 2019

shouldn't it also remove the clipboard.toml?

@kendonB
Copy link
Collaborator

kendonB commented Jun 21, 2019

I am no longer seeing the error though

@comodoro
Copy link
Contributor Author

@kendonB Sorry, yes, thanks, added. Note though that if you already have the path changed from trying this PR, it will not be touched.

@kendonB
Copy link
Collaborator

kendonB commented Jun 22, 2019

I updated this PR locally, changed my clipboard path back to the toml in settings, and now see the old clipboard file disappear. Great! I'm going to merge this.

@kendonB kendonB merged commit b5ba877 into dictation-toolbox:develop Jun 22, 2019
kendonB added a commit to kendonB/Caster that referenced this pull request Jun 23, 2019
kendonB added a commit to kendonB/Caster that referenced this pull request Jun 23, 2019
@LexiconCode
Copy link
Member

This error keeps popping back up. Looks like it's trying to read clipboard.json as Toml file.
@comodoro

<class 'toml.decoder.TomlDecodeError'>
Found invalid character in key name: '}'. Try quoting the key name. (line 1 column 2 char 1)
<traceback object at 0x1C43C170>
  File "D:\Backup\Library\Documents\Caster\castervoice\lib\utilities.py", line 113, in load_toml_file
    result = toml.loads(f.read())

  File "C:\Python27\lib\site-packages\toml\decoder.py", line 227, in loads
    original, i)


@comodoro
Copy link
Contributor Author

@LexiconCode Can happen now if developers switch branches with and without this (and the opposite, if you remake toml clipboard on an old branch). I think for users it should be safe.

@LexiconCode
Copy link
Member

That makes total sense.

@LexiconCode
Copy link
Member

@comodoro should we keep using json for clipboard?
Then remove the following temporary code for migrating

# Temporary piece of code to seamlessly migrate clipboards to JSON

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Caster Issues pertaining to primarily the Caster project. High Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants