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

Remove unnecessary WSR-specific code #643

Merged

Conversation

drmfinlay
Copy link
Member

This should close #554.

I've also fixed the recognize_forever() call in _caster.py (while loop not required).

As before, Nexus.history is not registered if running the tests
(when real_merger_config=False).
@LexiconCode LexiconCode added Enhancement Enhancement of an existing feature Windows Speech Recognition Windows Speech Recognition Backend labels Jul 11, 2019
@drmfinlay drmfinlay changed the title Remove RecognitionHistoryForWSR class and wsrdf.py file Remove unnecessary WSR-specific code Jul 11, 2019
@LexiconCode
Copy link
Member

LexiconCode commented Jul 11, 2019

@Danesprite Testing this would WSR and the record from history command.
It doubles the entry of the history per recognition.

For instance if I say the command maximize window once it's listed twice in record from history GUI.

@drmfinlay
Copy link
Member Author

Thanks for testing it. WSR behaves weirdly with recognition observers. It's a dragonfly problem which I've neglected to make a GitHub issue for. Will do that tomorrow.

I believe this behaviour would also occur with the RecognitionHistoryForWSR class, although I'm not sure if it was actually receiving any recognition history because it wasn't registered.

@LexiconCode
Copy link
Member

LexiconCode commented Jul 11, 2019

record from history command is implemented in castervoice\lib\ccr\recording\history.py. Looking under def record_from_history(self) at self.nexus.history[:] it grabs the history from the nexus which is RecognitionHistory(20) Therefore I think it most likely a issue with the observers with WRS. DNS does not display this issue.

@LexiconCode LexiconCode added the Waiting For Issue/PR Another open issue/PR (in caster or upstream) must be completed before this can be addressed. label Jul 11, 2019
@drmfinlay
Copy link
Member Author

Okay I can't reproduce the error I was talking about. Using git blame, I think I solved it a while ago in dictation-toolbox/dragonfly@e7392d2, although I didn't mention it.

In any case this is actually a Caster issue. The ContextStack class is adding the history for WSR again here:

if settings.WSR:
self._history.on_recognition(stack_item.get_preserved())

This is no longer necessary. There is another problem I've seen though: the again do / again <n> times commands aren't working.

@LexiconCode LexiconCode removed the Waiting For Issue/PR Another open issue/PR (in caster or upstream) must be completed before this can be addressed. label Jul 13, 2019
@LexiconCode
Copy link
Member

In any case this is actually a Caster issue. The ContextStack class is adding the history for WSR again here:

if settings.WSR:
self._history.on_recognition(stack_item.get_preserved())

I've tested removing the offending code. The record from history command works with both WSR and DNS. I've also verified that the ContextStack is still valid according to the unit tests.

@LexiconCode
Copy link
Member

Can you see if the main pull allows for edit from maintainers? I'm trying to figure out how to submit changes via push pull in git instead of the github UI.

@drmfinlay
Copy link
Member Author

drmfinlay commented Jul 15, 2019

Well that's good.

'Allow edits from maintainers' is ticked for me. Is your local repository pointed to [email protected]:Danesprite/caster.git or https://github.com/Danesprite/caster.git for Push/Fetch URLs? Should be able to test with git remote show origin.

It's no big deal if you can't get it working; I should be able to fix this in the next few days, it's just that these sort of changes to Caster take a while to test because of Caster's lengthy load time.

@LexiconCode
Copy link
Member

LexiconCode commented Jul 15, 2019

Thanks for explaining because this is kind of a pain point when contributing to anyone's pull request. I feel like I'm missing something obvious however here is the output of my attempts.

"C:\Program Files\Git\bin\git.exe" push --recurse-submodules=check --progress "Danesprite" refs/heads/Danesprite/remove-wsr-recog-history:refs/heads/Danesprite/remove-wsr-recog-history
Enumerating objects: 125, done.
Counting objects: 100% (111/111), done.
Delta compression using up to 4 threads
Compressing objects: 100% (87/87), done.
Writing objects: 100% (87/87), 19.54 KiB | 1.08 MiB/s, done.
Total 87 (delta 60), reused 0 (delta 0)
remote: Resolving deltas: 100% (60/60), completed with 17 local objects.
To https://github.com/Danesprite/caster.git
 ! [remote rejected]   Danesprite/remove-wsr-recog-history -> Danesprite/remove-wsr-recog-history (permission denied)
error: failed to push some refs to 'https://github.com/Danesprite/caster.git'
Done

"C:\Program Files\Git\bin\git.exe" push --recurse-submodules=check --progress "Danesprite" refs/heads/Danesprite/remove-wsr-recog-history:refs/heads/Danesprite/remove-wsr-recog-history
fatal: remote error:
  You can't push to git://github.com/Danesprite/caster.git
  Use https://github.com/Danesprite/caster.git
Done

I've added your repository as a remote named "Danesprite" as I reserve origin as a reference to my repository.

git remote show Danesprite
* remote Danesprite
  Fetch URL: git://github.com/Danesprite/caster.git
  Push  URL: git://github.com/Danesprite/caster.git
  HEAD branch: master
  Remote branches:
    gh-pages                 new (next fetch will store in remotes/Danesprite)
    master                   new (next fetch will store in remotes/Danesprite)
    remove-wsr-recog-history tracked
  Local ref configured for 'git push':
    master pushes to master (fast-forwardable)

@drmfinlay
Copy link
Member Author

Hmm okay then. I'm not sure why the git:// URL isn't working. Maybe try the following commands:

# Set the remote URL to use HTTPS.
git remote set-url Danesprite https://github.com/Danesprite/caster.git

# Push to Danesprite/remove-wsr-recog-history
git push Danesprite remove-wsr-recog-history

If you use SSH, then you use [email protected]:Danesprite/caster.git instead of the https:// URL. You also must have your public key in the GitHub settings: https://github.com/settings/keys

@LexiconCode
Copy link
Member

LexiconCode commented Jul 16, 2019

The issue I was having was related to the default push repository and branch. A bit confusing as it was expressly defined in the git command so that shouldn't matter. However I now know the method for making this work at least within Git Extensions.

Thanks for all your advice @Danesprite

@LexiconCode LexiconCode merged commit 1033702 into dictation-toolbox:master Jul 16, 2019
@drmfinlay
Copy link
Member Author

Ah, okay then. No worries, I'm glad you got it sorted out 👍

@drmfinlay drmfinlay deleted the remove-wsr-recog-history branch July 16, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhancement of an existing feature Windows Speech Recognition Windows Speech Recognition Backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update some engine-specific code
2 participants