plugins: Add Pitivi Developer Console plugin
Needs ReviewPublic

Authored by cfoch on Jun 2 2017, 8:28 PM.

Details

Maniphest Tasks
T7784: Developer Console
Reviewers
thiblahute
aleb
Summary

Depends on D1841

Diff Detail

Repository
rPTV Pitivi
Branch
developer-console-code-module-phabricator
There are a very large number of changes, so older changes are hidden. Show Older Changes
aleb requested changes to this revision.Aug 25 2017, 11:10 PM
aleb added inline comments.
plugins/console/console.py
92

Maybe file a bug in GTK.
You could return what the method returns. :)

plugins/console/widgets.py
39

The console can be used to access an app, window, or anything through the provided namespace. It works by redirecting...

41

*independent

86

some more ' to be replaced with ". Please Ctrl+F

173

Makes sense, but the names can be confusing. We should see how to reorganize/rename them.

261

remove empty line at start of method

336

I meant here the "i" variable should be "end_iter". The name of the method was fine, and the new name does not even make sense.

This revision now requires changes to proceed.Aug 25 2017, 11:10 PM
aleb added inline comments.Aug 25 2017, 11:37 PM
plugins/console/widgets.py
179

I tried to remove "if not event_state: buf.place_cursor(inp)" and it works fine, so let's remove this, and then we can rename the method to __is_cursor_at_start """Returns whether the prompt is exactly after the prompt.""". Maybe rename "input" to "after-prompt". And maybe inp to ..after_prompt?

190

*inp/*cur, to be consistent with the other methods.

aleb added inline comments.Aug 25 2017, 11:58 PM
plugins/console/widgets.py
65

Add _tag suffix for these fields

134

cur -> end ?

155

You could catch "SyntaxError: unexpected EOF while parsing" and then also self.prompt = ConsoleWidget.PROMPT_INDENT...

>>> a(
Traceback (most recent call last):
  File "/home/aleb/dev/pitivi-cfoch/pitivi/plugins/console/widgets.py", line 507, in __run
    eval_result = eval(command, global_namespace, local_namespace)
  File "<string>", line 1
    a(
     ^
SyntaxError: unexpected EOF while parsing
cfoch added inline comments.Aug 26 2017, 2:54 PM
plugins/console/widgets.py
179

Do I rename "inp" to "after_prompt" here or in all the code?

cfoch added inline comments.Aug 26 2017, 4:42 PM
plugins/console/widgets.py
155

I am not sure what to do here. I can catch SyntaxError but not the message. Maybe it should insert the PROMP_INDENT (" ...") ?

cfoch added inline comments.Aug 26 2017, 4:46 PM
plugins/console/widgets.py
190

Why? I am not sure what to change here and why to do the change.
I am not sure if you refer to "ins" or to "cur". It is "it" because it refers to "iter", it is "ins" because it refers to the "insert" mark.

aleb added inline comments.Aug 26 2017, 7:38 PM
plugins/console/widgets.py
39

"It works by redirecting." ?

155

I don't understand why you ask that.
See in https://docs.python.org/3/library/parser.html and in the ast module - maybe you can parse it first and if it's "not complete", change the prompt to PROMPT_INDENT..?
See also https://docs.python.org/3/library/functions.html?highlight=syntaxerror#compile

171

I suggested this but "the prompt is exactly after the prompt" sounds confusing. Maybe "cursor" ?

179

I'm just thinking that the inp name for the local variable does not make sense. If you change it, see maybe it makes sense to do the same rename in other places. If you don't like after-prompt, let's find some other name. command_line_start ?

190

Look for buf.get_iter_at_mark(buf.get_mark("input")) and maybe use the same variable name to store the result.

aleb requested changes to this revision.Aug 27 2017, 10:47 AM
aleb added inline comments.
plugins/console/utils.py
33

wrap in a try/finally, to make sure sys.stdout is reverted if an exception is raised

35

You don't need to swap them, only set sys.stdout.

plugins/console/widgets.py
39

Read the whole paragraph, you should remove the entire text I quoted.

50

You can use sys.ps1 and sys.ps2 instead of these!

136

I think we can get rid of ctrl+return with this change!

322

This is a private method, no need to document it like this. Instead, I would add a comment in the code, to make it clear what's happening.

326

You can return directly, no need to use res

328

I think we should not have to call runcode. See how https://docs.python.org/3/library/code.html#code.InteractiveConsole.push works. It tries to run the buffered lines whenever you push, and if it succeeds it resets the buffer!

This revision now requires changes to proceed.Aug 27 2017, 10:47 AM
cfoch marked an inline comment as done.Aug 27 2017, 4:44 PM
cfoch added inline comments.
plugins/console/widgets.py
328

That's because the Python console only accept lines. That bothers me TBH. If you want to use push with more than one line you will get an error.
But "runcode" doesn't have this problem. This is useful when you want to copy&paste some chunk of code (multiple lines) into the Developer Console.

aleb requested changes to this revision.Aug 27 2017, 10:34 PM
aleb added inline comments.
plugins/console/widgets.py
22

This is not useful info for the user of this widget. Please remove.

47

Why is this needed? Remove?

64

We don't load any settings here.

67

Remove this check, make namespace mandatory, since we'll always use it, and explain it in the "Args:" section in the class pydoc.

93
  1. unused parameter can be removed
  2. You can also maybe use self.__view so it's clear what view it is.

Same for __is_cursor_at_start

124

Maybe line_start_mark is more clear?

137

I imagine it might be useful when editing multi-line commands, but this is too complex. I vote for removing it. The standard Python console does not do anything fancy, for example.

158

Let's remove this and don't bother about spaces at the end?

195

Why is this necessary? Why isn't the fact that we return True not enough? Add a comment or remove the line. See also above same thing.

197

Move this call into history_up, same for history_down?

This revision now requires changes to proceed.Aug 27 2017, 10:34 PM
cfoch added inline comments.Aug 27 2017, 11:55 PM
plugins/console/widgets.py
124

for everything?

aleb added inline comments.Aug 28 2017, 8:51 AM
plugins/console/widgets.py
124

Yes, for everything. If you only change it in one place you'd get inconsistent variable names..

How about: prompt_start_mark and command_start_mark?

cfoch added inline comments.Aug 28 2017, 9:19 AM
plugins/console/widgets.py
124

All the new proposed names are fine to me. Please choose the one you like the most :)

cfoch updated this revision to Diff 6422.Aug 28 2017, 8:55 PM

Missing if replacing after-prompt by command-start or prompt-start