psethwick / plover_console_ui

Text User Interface plugin for Plover
GNU General Public License v3.0
20 stars 2 forks source link

[Feature request] Add-translation #10

Closed user202729 closed 3 years ago

user202729 commented 3 years ago

Since this isn't supposed to be able to open new windows, there are 2 options:

psethwick commented 3 years ago

It exists! It is just undocumented (and probably under-tested)

Currently the only entry point is {PLOVER:ADD_TRANSLATION}, Adding a console command is relatively trivial, might add as 'addtranslation' at the top level to avoid confusion with dictionary add

It behaves similarly to the gui_qt one:

psethwick commented 3 years ago

I'm planning much more documentation

user202729 commented 3 years ago

Okay... (I did try using that, I used to do that all the time. It doubles as the lookup tool in the qt GUI)

│ERROR: hook 'add_translation' callback functools.partial(<bound method ConsoleLayout.on_add_translation of <plo│
│ver_console_ui.layout.ConsoleLayout object at 0x7fe4a76463a0>>, <ConsoleEngine(Thread-1-engine, started 1406200│
│31727168)>) failed                                                                                             │
│struct.error: required argument is not an integer    

The error message was NoneType cannot be called, so I assumed that it's unimplemented.

... there's no traceback?

Update: I see, plover.log.error calls logging.Logger.error which doesn't show the traceback. Perhaps intentionally.

Update: it looks like that the error is not that of this plugin. Investigating.

psethwick commented 3 years ago

Ah, hm. It did work. I'll have a look; it is probably something dumb I did in a refactoring.

user202729 commented 3 years ago

Okay, the issue is that my WM doesn't support something (focus_console() ), and the error thrown breaks the whole thing.

Just add a if self.console is not None?

user202729 commented 3 years ago

Update: adding that does work (I remember that there are some similar error handling code in Plover too, which swallow the error messages)

--- a/plover_console_ui/focus.py
+++ b/plover_console_ui/focus.py
@@ -21,11 +21,13 @@ class Focus:
         self.prev = GetForegroundWindow()

     def focus_prev(self):
-        SetForegroundWindow(self.prev)
+        if self.prev is not None:
+            SetForegroundWindow(self.prev)

     def focus_console(self):
         self.set_prev()
-        SetForegroundWindow(self.console)
+        if self.console is not None:
+            SetForegroundWindow(self.console)

 focus = Focus()

The self.prev is not None check appears to be unnecessary (not proven).

... "No newline at end of file" (vim automatically add one, and seeing that in git diff is a little annoying; although other people with editor that automatically format code might complain about other things too, so it's up to you)?

More bugs

psethwick commented 3 years ago

How do I exit the add translation without adding anything?

escape key should do this

The "Strokes: / Output:" prompt doesn't show if Plover has just been opened (no previous translation)

I can't reproduce this, are there more steps?

focus.console being None makes sense (and I will add the None check) The only way I'm assigning it is grabbing the output of GetForegroundWindow on start-up so if xterm (or similar) isn't actually focused in your window manager it won't work :(

I'd love to find the right window, but doing so in a cross-platform way seems like trouble

user202729 commented 3 years ago

Perhaps the "escape doesn't work" error comes from this error too.

Normally, the escape is still delayed by about half a second. I don't see any CPU usage, however.

(could be some prompt_toolkit unintended feature/wrong key binding time-out setting, because escape can be sent by several other combinations too)

Edit: indeed it is.

Application.ttimeoutlen: Like Vim’s ttimeoutlen option. When to flush the input (For flushing escape keys.) This is important on terminals that use vt100 input. We can’t distinguish the escape key from for instance the left-arrow key, if we don’t know what follows after “x1b”. This little timer will consider “x1b” to be escape if nothing did follow in this time span. This seems to work like the ttimeoutlen option in Vim.

psethwick commented 3 years ago

Yeah, I experienced the timoutlen thing already.

should be working now (with a snappy esc, even)

The bug was: not running application.invalidate() on the add_translation hook. Left prompt_toolkit in a somewhat odd state, I am supposing.

I didn't experience because: invalidate() gets call on buffer updates (e.g. tape/suggestions were on...)

psethwick commented 3 years ago

perhaps escape might be a bad choice for exit, because of the vt100 thing

user202729 commented 3 years ago

Something else...

Sometimes (because outputs are automatically taken from the last strokes), the "output" field is populated with word\n\n (literal newlines), and those new lines can't be seen except in the console.

I think the qt version allows entering literal string \n in the box, but literal \n are removed.


There's also a case that the filter is still in place after the add-translation prompt is exited. I still can't reproduce it.

psethwick commented 3 years ago

Newlines should be easy enough (I have not scoured the Qt code very much)

I have a few ideas on how the filter removal may not happen, will try and address all of them

user202729 commented 3 years ago

In case you didn't figure it out, it happens when I press TKUPT twice then cancel it. (of course there's no way to cancel it twice)

psethwick commented 3 years ago

That was my guess, actually! I have pushed some changes, now it checks to see if a filter is there before adding it. Newlines are now removed from the pre-populated translation.

There were also some other theoretical ways the filter could've stuck around (example: TKUPT, then the stroke for {PLOVER:LOOKUP}. I think I've found them all, but could have missed something.

user202729 commented 3 years ago

Would that interfere badly with other plugins that use filter too (if any)?

Would be better to store the state internally.

psethwick commented 3 years ago

The actual implementation isn't 'remove all filters', it's if this specific function ref is in the filters then remove it.

Perhaps would be better to hold the state internally, but it shouldn't interfere

user202729 commented 3 years ago

Related: there's no way to enter a literal \n. (same for \t)

Perhaps ^V\n?

Although this one is better than the qt add translation feature that it handles backslashes consistently (always verbatim)

psethwick commented 3 years ago

Interesting, I'll try and come up with something ^V could work

user202729 commented 3 years ago

Re stripping \n, should trailing spaces be stripped too?

psethwick commented 3 years ago

they probably should! (also.. I guess they are currently not)

edit: I threw an .rstrip() in there, should be now!

(still need to address some way of entering \n etc)