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

[ENH] OWPythonScript: Better text editor #5208

Merged
merged 24 commits into from
Aug 18, 2021

Conversation

irgolic
Copy link
Member

@irgolic irgolic commented Jan 24, 2021

Requires #5526 and #5530.

OWPythonScript PRs:

here -> #5228

Description of changes

Screenshot 2021-01-24 at 18 14 36
I've separated the Python Script rewrite into several PRs. Here's the first, and probably biggest. :) Coming soon are a PR replacing the console, a PR replacing the library, and a PR integrating pandas.

Don't fret the number of added lines, there's hundreds of tests.

In this PR, andreikop/Qutepart is ported into Orange and adjusted for the python script widget.

Also, a fake function signature adorns the editor, showing what variables are on the input. A return statement in the script will be used for variable collection in a later PR, when that's reworked. A later PR will also include completions in the editor.

RFC because please give it a try, let me know if any keyboard shortcuts/vim mode should be adjusted.

Includes
  • Code changes
  • Tests
  • Documentation

@irgolic irgolic force-pushed the pythonscript-editor branch 4 times, most recently from 24c53a6 to 5f726e0 Compare January 24, 2021 23:58
@codecov
Copy link

codecov bot commented Jan 25, 2021

Codecov Report

Merging #5208 (124b050) into master (1d3c728) will decrease coverage by 0.48%.
The diff coverage is 72.78%.

@@            Coverage Diff             @@
##           master    #5208      +/-   ##
==========================================
- Coverage   86.40%   85.91%   -0.49%     
==========================================
  Files         304      313       +9     
  Lines       61826    65315    +3489     
==========================================
+ Hits        53419    56114    +2695     
- Misses       8407     9201     +794     

@irgolic irgolic force-pushed the pythonscript-editor branch from 5f726e0 to 628c005 Compare January 25, 2021 00:30
@markotoplak
Copy link
Member

markotoplak commented Jan 25, 2021

I am against merging this into Orange, because in my opinion, there is too much code for the benefit. Instead, we should encourage people to write/test their scripts in a real python dev tool.

Or are we really writing an editing kit? Should we add a debugger? Profiler?

@borondics
Copy link
Member

I think the Python Script widget is an important one to enable quick and dirty, proof-of-principle implementations for new functionality.

While I would really like some improvements in the Python Script widget I am not entirely sure if adding the Vim mode is that helpful. Or maybe it is if autocomplete can be enabled/added because of this change? That would be an important one in my opinion because there is a big barrier for novice programmers to understand even what the data structure is in Orange.

@markotoplak the potential issue with encouraging people to using external tools for script development is that not all people are capable of doing that who would be able to use the Python Script widget for doing the job...

@ajdapretnar
Copy link
Contributor

I agree that Python Script has its place in Orange and that it needs some improvements. I very, very often use it for some small preprocessing task for text mining and it doesn't really make sense to go out of Orange and than back in.

Python Script most urgently needs code completion, because it is a nightmare to use otherwise. The rest is optional.

@markotoplak
Copy link
Member

@markotoplak the potential issue with encouraging people to using external tools for script development is that not all people are capable of doing that who would be able to use the Python Script widget for doing the job...

I think that people who can not use Python externally also cannot use Python Script widget for development. For running code, sure, but for development of new stuff, forget it.

@irgolic
Copy link
Member Author

irgolic commented Jan 25, 2021

I'd like to remind everyone that pretty much everything this PR (and the following) adds is optional. The current python script is fully sufficient for any and all python tasks. However, its editor is unintuitive (a simple QPlainTextEdit with no line numbers, no support for complex interactions), its console is basic (no colors, freezes when running anything), its exposed interface is mystifying to new Orange users (doesn't easily expose numpy arrays or pandas DataFrames), and the library is confusing (doesn't actually save files to disk, and it's a UI nightmare).

Our biggest critique is how bad our python script is. Questionnaires have shown that over half of Orange's users know how to use python, over a quarter are familiar with pandas. I want to build for them a fun, intuitive, engaging, and forgiving environment, where they can play with python, without the fear of crashing Orange, without Orange freezing, using their favorite text interaction methods (vim mode, rectangular selection ...), in a familiar color scheme (adopted from jupyter's colors), with an intuitive script library.

I am against merging this into Orange, because in my opinion, there is too much code for the benefit.

These are this PR's changes:

 Orange/widgets/data/owpythonscript.py                                      |  365 ++++++++++-----
 Orange/widgets/data/tests/test_owpythonscript.py                           |   89 ++--
 Orange/widgets/data/utils/pythoneditor/__init__.py                         |    0
 Orange/widgets/data/utils/pythoneditor/brackethighlighter.py               |  158 +++++++
 Orange/widgets/data/utils/pythoneditor/completer.py                        |  578 +++++++++++++++++++++++
 Orange/widgets/data/utils/pythoneditor/editor.py                           | 1828 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 Orange/widgets/data/utils/pythoneditor/indenter.py                         |  530 +++++++++++++++++++++
 Orange/widgets/data/utils/pythoneditor/lines.py                            |  189 ++++++++
 Orange/widgets/data/utils/pythoneditor/rectangularselection.py             |  263 +++++++++++
 Orange/widgets/data/utils/pythoneditor/tests/__init__.py                   |    0
 Orange/widgets/data/utils/pythoneditor/tests/base.py                       |   79 ++++
 Orange/widgets/data/utils/pythoneditor/tests/run_all.py                    |   27 ++
 Orange/widgets/data/utils/pythoneditor/tests/test_api.py                   |  281 ++++++++++++
 Orange/widgets/data/utils/pythoneditor/tests/test_bracket_highlighter.py   |   71 +++
 Orange/widgets/data/utils/pythoneditor/tests/test_draw_whitespace.py       |  102 +++++
 Orange/widgets/data/utils/pythoneditor/tests/test_edit.py                  |  111 +++++
 Orange/widgets/data/utils/pythoneditor/tests/test_indent.py                |  140 ++++++
 Orange/widgets/data/utils/pythoneditor/tests/test_indenter/__init__.py     |    9 +
 Orange/widgets/data/utils/pythoneditor/tests/test_indenter/indenttest.py   |   68 +++
 Orange/widgets/data/utils/pythoneditor/tests/test_indenter/test_python.py  |  342 ++++++++++++++
 Orange/widgets/data/utils/pythoneditor/tests/test_rectangular_selection.py |  259 +++++++++++
 Orange/widgets/data/utils/pythoneditor/tests/test_vim.py                   | 1041 ++++++++++++++++++++++++++++++++++++++++++
 Orange/widgets/data/utils/pythoneditor/vim.py                              | 1287 +++++++++++++++++++++++++++++++++++++++++++++++++++
 doc/widgets.json                                                           |    1 -
 requirements-gui.txt                                                       |    1 +

2500 lines of code (a third) are tests. Implementing a good editor just takes a lot of code. There's features like rectangular selection, completions (integrating with the console in next PR), python syntax legibility, vim mode, all kinds of shortcuts...

I'll make sure our coverage doesn't drop steeply.

Or are we really writing an editing kit? Should we add a debugger? Profiler?
Instead, we should encourage people to write/test their scripts in a real python dev tool.

I think Python Script is the most important widget in our catalog, and it deserves some love.

I've genuinely considered developing a debugger as my master's project, or a summer project. Do you suggest we remove the editor altogether? What's your workflow for testing and running them with Orange inputs?

I think that people who can not use Python externally also cannot use Python Script widget for development. For running code, sure, but for development of new stuff, forget it.

I'd love to make it possible for people to run some random pandas code they find on the internet as part of an Orange workflow :)

@markotoplak
Copy link
Member

An editor packaged into a separate python module that could be easily updated to upstream editor would be much preferable.

I've genuinely considered developing a debugger as my master's project, or a summer project.

Sure, play with it, make a prototype, but please do not put it in Orange. We already have too much code to maintain and the best we could do here to make another sub-par dev environment.

Do you suggest we remove the editor altogether? What's your workflow for testing and running them with Orange inputs?

  • Save inputs to files (not needed always, I usually make scripts on generated or standard data)
  • Create a new file in vim/PyCharm.
  • Edit/debug.
  • Copy into the Python Script widget and perhaps adjust inputs/outputs.

@borondics
Copy link
Member

@markotoplak if fully support and appreciate your caution with the code maintenance!! I think this is a very serious issue with a large project like Orange and I agree that the best additions are the easily maintainable ones.

Your bullet list for the python script development is really not feasible for a non-programmer though. :)

@markotoplak
Copy link
Member

markotoplak commented Jan 25, 2021

Your bullet list for the python script development is really not feasible for a non-programmer though. :)

Yes, I know. But don't think a better editor would help. To write code, you need to know how to program. The best editor can provide is good code completion, which would allow you to be slightly faster, but only if you already know what your doing. And if you already know what you are doing, then you also know that directly editing code outside your preferred environment is not what you should be doing at all.

So I think we need to focus on programmers with this widget. What they probably need is better understanding of what inputs/outputs are and how to handle them: we would need good docs with examples and also use established ways of accessing data (such as @irgolic proposes in another PR). Code completion would help very little here.

@borondics
Copy link
Member

I think the two unproblematic groups are the people who know nothing about programming and the ones who are good programmers. Neither of those need changes to the Python Script widget. The first group will never use it and for the second it is already OK.

The problematic group is in the middle. If you want to help unexperienced but interested people, then improvements are needed. I would also say that the editor is not too important, but I agree with @ajdapretnar that code completion would be extremely useful.

@markotoplak
Copy link
Member

We discussed this with @irgolic, @lanzagar, @VesnaT and @JakaKokosar. We decided to go forward with this PR and further console improvements that will also bring code completion and non-blocking execution into the widget.

We decided to put the new editor within orange3 repository so that we won't need to do synchronized separate releases when something changes.

I do not remember if we said anything about the Vim mode. Do we keep it?

@irgolic
Copy link
Member Author

irgolic commented Jan 29, 2021

I do not remember if we said anything about the Vim mode. Do we keep it?

Yeah, why not? :P I'll make a meme about it too and we'll post it to Facebook.

@irgolic irgolic mentioned this pull request Jan 31, 2021
3 tasks
@markotoplak markotoplak changed the title [RFC] OWPythonScript: Replace text editor [ENH] OWPythonScript: Better text editor Feb 1, 2021
@markotoplak markotoplak added the merge after release Potentially unstable and needs to be tested well. label Feb 1, 2021
@irgolic irgolic force-pushed the pythonscript-editor branch from 4ac8089 to 20a9384 Compare February 1, 2021 16:57
@PrimozGodec
Copy link
Contributor

@irgolic can you rebase this PR to master?

@irgolic irgolic removed the merge after release Potentially unstable and needs to be tested well. label Mar 16, 2021
@irgolic irgolic force-pushed the pythonscript-editor branch 2 times, most recently from 2ffbf1f to 0a24861 Compare March 16, 2021 20:01
@PrimozGodec
Copy link
Contributor

PrimozGodec commented Mar 19, 2021

When I enable the Vim mode normal label takes more space than the is the current width of the controlArea and on my computer it was not raised but rather part of it was cut away.

Screenshot 2021-03-19 at 08 53 24

@PrimozGodec
Copy link
Contributor

PrimozGodec commented Mar 19, 2021

Maybe it can be fixed later but I just want to let you know that current color selection is not compatible with the dark mode. The blue color of python_scritp() and the line numbers are difficult to read. Other colors are compatible.

Screenshot 2021-03-19 at 08 57 02

@PrimozGodec
Copy link
Contributor

@irgolic can you also check why Windows tests are failing?

@irgolic irgolic force-pushed the pythonscript-editor branch from 7cbf01f to 439f030 Compare July 17, 2021 00:34
@PrimozGodec
Copy link
Contributor

PrimozGodec commented Aug 11, 2021

@irgolic may you just resolve conflicts here, please?

@irgolic irgolic force-pushed the pythonscript-editor branch from 439f030 to 70de44e Compare August 12, 2021 18:33
@PrimozGodec PrimozGodec merged commit 4b5e43c into biolab:master Aug 18, 2021
@PrimozGodec
Copy link
Contributor

It looks good now. I like the new editor 👍

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 this pull request may close these issues.

5 participants