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

[ui] Python Script Editor Improvements #2587

Merged
merged 10 commits into from
Jan 10, 2025
Merged

Conversation

waaake
Copy link
Contributor

@waaake waaake commented Oct 28, 2024

Description

ScriptEditor Improvements

Features list

  • Script Editor now retains history of the code which is executed across Meshroom sessions.
  • Script Editor also highlights the code Syntax for Python
  • The output clearly distinguishes between executed code and the result of the code with different colors.

@waaake waaake self-assigned this Oct 28, 2024
@cbentejac cbentejac added this to the Meshroom 2024.1.0 milestone Oct 28, 2024
horizontalAlignment: Text.AlignHCenter
Layout.fillWidth: true
}
MSplitView {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be discussed but I wonder if it wouldn't be better to have the split view be horizontal instead of vertical... As such, it's is pretty much unusable unless we expand the Script Editor's tab, which may not be ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback @cbentejac, I do agree with how the widgets are placed in Meshroom at the moment, the best layout is a RowLayout.
Have reverted back in terms of Layout and I guess we can keep it till the time we have a better location/placement of Script Editor.

@waaake waaake marked this pull request as draft November 26, 2024 11:28
@waaake waaake force-pushed the dev/PythonScriptEditor branch from 2bd93ff to 52b52b8 Compare November 26, 2024 14:47
@waaake waaake marked this pull request as ready for review November 26, 2024 14:48
@waaake waaake force-pushed the dev/PythonScriptEditor branch from 52b52b8 to dd40d93 Compare December 12, 2024 15:33
@waaake waaake requested a review from yann-lty December 12, 2024 15:34
@waaake
Copy link
Contributor Author

waaake commented Dec 12, 2024

Thanks for the suggestions @yann-lty
I have added a confirmation dialog for clearing the history, let me know how these new icons feel.
MeshroomScriptEditorConfirmation

@waaake waaake force-pushed the dev/PythonScriptEditor branch from dd40d93 to d0ecb8a Compare December 12, 2024 15:38
waaake added 6 commits January 7, 2025 09:25
…xceptions

ScriptEditorManager now also allows the code to be saved and retrieved back. Exceptions are now shown with a better output to the user.
ScriptEditor is now part of a ColumnLayout in an MSplitView allowing more control over what is being viewed.
Python syntax within the script editor is now highlighted making it easier to understand and write smaller code in it.
A Row Layout is more practical for using script editor with the current placement of meshroom GUI.
Updated Icons for ScriptEditor

Script Editor shows a confirmation dialog before clearing history
@waaake waaake force-pushed the dev/PythonScriptEditor branch from d0ecb8a to 49052df Compare January 7, 2025 04:08
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.93%. Comparing base (e43cd62) to head (7384db8).
Report is 23 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2587   +/-   ##
========================================
  Coverage    69.93%   69.93%           
========================================
  Files          121      121           
  Lines         7088     7088           
========================================
  Hits          4957     4957           
  Misses        2131     2131           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

*/

// Replace the text to be RichText Supportive
return "<font color=#49a1f3>" + "Result: " + replace(text, "\n", "<br>") + "</font><br><br>"
Copy link
Member

@fabiencastan fabiencastan Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "<font color=#49a1f3>" + "Result: " + replace(text, "\n", "<br>") + "</font><br><br>"
return "<font color=#49a1f3>> Result:<br>" + replace(text, "\n", "<br>") + "</font><br>"

*/

// Replace the text to be RichText Supportive
return "<font color=#868686>" + replace(text, "\n", "<br>") + "</font><br><br>"
Copy link
Member

@fabiencastan fabiencastan Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return "<font color=#868686>" + replace(text, "\n", "<br>") + "</font><br><br>"
return "<font color=#868686>>Input:<br>" + replace(text, "\n", "<br>") + "</font><br>"

Copy link
Member

@fabiencastan fabiencastan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot scroll to see the end of the text:
image

…flickables

Formatted the input and output text for output display text area
@fabiencastan
Copy link
Member

font size for icons is too big:
image

@fabiencastan
Copy link
Member

On execution, we should scroll the output to the end.

@fabiencastan
Copy link
Member

I would propose to update the order of the icons:
image
Groups all the actions related to the script, center the execution, and align to the right the clear.

Or:
image

@fabiencastan
Copy link
Member

Could it be a SplitView between the Editor and the Output.

@fabiencastan
Copy link
Member

Maybe also disable the button if there is no action to perform on the "Get Previous/Next Script".

@fabiencastan
Copy link
Member

The first click on "Get Previous Script" is not working. You need to click a second time to do the action.

@waaake
Copy link
Contributor Author

waaake commented Jan 10, 2025

The first click on "Get Previous Script" is not working. You need to click a second time to do the action.

It feels like that actually, but it is actually working,
If I executed the below example:
print("A")

Then I executed
print("B")
Clicked on Get Previous Script,
it gets back print("B")

but I would not notice any difference because my current text in script editor input is still print("B") so I have to click on it two times to get it back to print("A")

I think in the first version of script editor we used to clear the input as soon as it was executed so this made it work as expected but now that we're not clearing the script contents, it feels like this is not working....
🙁

@fabiencastan
Copy link
Member

It would be good to scroll the output to the end on execution.

@fabiencastan
Copy link
Member

fabiencastan commented Jan 10, 2025

It looks surprising to me that clearing the history is not only clearing the history but also the current code in the script editor.

But if I close and re-open Meshroom, this code is back magically (so the history has not been fully cleared). If we do not remove the current code, then it would be fine to have it when you close and re-open.

Layout.fillHeight: true
width: root.width
MSplitView {
id: scriptSplitView;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we do not put the extra ";" in general.

@fabiencastan fabiencastan merged commit 3e8b736 into develop Jan 10, 2025
5 checks passed
@fabiencastan fabiencastan deleted the dev/PythonScriptEditor branch January 10, 2025 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants