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

Overhaul Editor doc with many new sections and screenshots #331

Merged
merged 30 commits into from
Nov 15, 2022

Conversation

hyounes4560
Copy link
Contributor

Pull Request

Pull Request Checklist

  • Read and followed this repo's Contributing Guidelines
  • Based your PR on the latest version of the correct branch (master or 4.x)
  • Checked your writing carefully for correct English spelling, grammar, etc
  • Described your changes and the motivation for them below
  • Noted what issue(s) this pull request resolves, creating one if needed

Description of Changes

  • Add new content about some of the Editor's main features
  • Add the keyboard shortcut section
  • Mention how to run cells Mention how to run cells #97
  • Use some existing images

This is still WIP/draft as I am still working on the rest of the images/GIFs. I used the Image/Image Placeholder test instead of the actual images for now.

Issue(s) Resolved

Fixes #97

Copy link
Member

@steff456 steff456 left a comment

Choose a reason for hiding this comment

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

Thanks for this amazing work @hyounes4560 ! I left some comments, mainly there are some issues with the rendering and consistency among the way shortcuts are displayed.

doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
hyounes4560 and others added 22 commits October 11, 2022 19:25
@hyounes4560
Copy link
Contributor Author

@CAM-Gerlach I don't have any changes locally - whatever exists in this PR is all I have; didn't add any new stuff.

I see, and indeed what you pushed confirms that. In that case, you can just reset your branch to the remote version directly:

git switch --force HY-wip-update-editor-docs
git fetch origin
git reset --hard origin/HY-wip-update-editor-docs

Thank you; this worked.

and then you'll be synced up and good to go for future commits/pushes.

@CAM-Gerlach I don't have anything else to push; the only missing thing here is some GIFs. I've shared the videos with you and as per a previous discussion, you could convert them to GIFs. Please let me know if you haven't received them Thanks again

@CAM-Gerlach
Copy link
Member

Thank you; this worked.

Great!

I don't have anything else to push; the only missing thing here is some GIFs. I've shared the videos with you and as per a previous discussion, you could convert them to GIFs. Please let me know if you haven't received them Thanks again

Oh, sorry about that! I had just gotten back from the Python Core Developer Sprint and was heavily backlogged in the aftermath of that, and got myself mixed up somehow thinking that this PR was pending the remaining screenshots/GIFs, rather than waiting on me to process them. My bad!

In any case, I have and will be very busy for the next week preparing for the big Python 3.11 release next Monday (I'm in charge of the What's New in Python 3.11 and also making sure everything is documented for the release, so I can't promise I will be able to find the spare time until then, but I'll do my best. Otherwise, I'll take care of it perhaps around the middle of next week, if that's okay. Thanks again for all your hard work, and looking forward to seeing this land soon!

@hyounes4560
Copy link
Contributor Author

Thank you; this worked.

Great!

I don't have anything else to push; the only missing thing here is some GIFs. I've shared the videos with you and as per a previous discussion, you could convert them to GIFs. Please let me know if you haven't received them Thanks again

Oh, sorry about that! I had just gotten back from the Python Core Developer Sprint and was heavily backlogged in the aftermath of that, and got myself mixed up somehow thinking that this PR was pending the remaining screenshots/GIFs, rather than waiting on me to process them. My bad!

In any case, I have and will be very busy for the next week preparing for the big Python 3.11 release next Monday (I'm in charge of the What's New in Python 3.11 and also making sure everything is documented for the release, so I can't promise I will be able to find the spare time until then, but I'll do my best. Otherwise, I'll take care of it perhaps around the middle of next week, if that's okay. Thanks again for all your hard work, and looking forward to seeing this land soon!

@CAM-Gerlach no worries; take your time.
Meanwhile, I'll try again to create a better quality GIFs as the current ones have quality issues and they're not clear. I will update the PR if I was successful; and you can take a look when you have time. Thanks again.

@CAM-Gerlach CAM-Gerlach dismissed steff456’s stale review October 22, 2022 00:32

Review comments were addressed

@steff456
Copy link
Member

@ccordoba12 and @dalthviz can you please do the final review to see if we can merge this PR by next week? Thanks!!

@CAM-Gerlach CAM-Gerlach self-requested a review November 13, 2022 02:31
@CAM-Gerlach CAM-Gerlach force-pushed the HY-wip-update-editor-docs branch from 487200c to c3dda6e Compare November 13, 2022 08:39
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Thanks again for your hard work on this @hyounes4560 ; the GIFs look pretty good—I just had to reprocess them to fix their size and other issues (see important note below). With that, LGTM now from me!

@CAM-Gerlach
Copy link
Member

⚠️ Important ⚠️

Hey @hyounes4560 , I just want to make sure you see this ASAP to avoid any Git issues. Before doing anything else, other than stashing any uncommitted changes (which this will replace), please run the following command to update your branch to the remove version:

git fetch origin && git rebase origin/HY-wip-update-editor-docs

This is due to the extraordinary step of force-pushing your branch with the reprocessed GIFs, for the reasons discussed below. I normally would never do that to a contributor's branch, even for those like you who have given me prior permission, but I wanted to avoid any risk of this branch accidentally being merged (rather than squashed) to the main branch of this repo, which would have substantial irreversible consequences, as well as avoid this keeping this branch inflated in size and making them unviewable in (or even crashing) the GitHub Files UI.


Thanks again for your hard work on this @hyounes4560 ; the GIFs look pretty good. There was just one blocking issue with them—they were huge 🙂 (the largest one, at 47 MB, was half the size of all >200 images and GIFs currently used in our docs combined), which would of course create correspondingly-sized problems both for docs writers and readers. To make a long story short, I've heavily optimized them to make them an average of 18x smaller (nearly 50x for the largest one) with only modest quality compromises, plus fixed a few other issues, and (force) pushed the changes to the PR (for the reasons above). If you'd like the full details of what I did, and a CLI "script" making it easier for me, you or others to do the same in the future, read on:

FULL DETAILS

If these were to be used as-is, the page would take around 15 seconds or more to load on even a 100 Mbit/s connection, or over two minutes at 10 Mbit/s. Furthermore, once committed to this repo, they would take up space forever, and everyone who tried to clone or pull it would need to download them, even if they were later replaced. They also won't reliably display in GitHub's files view, and take several seconds to tens of seconds to load in my local image viewer, sometimes even crashing it.

Therefore, I've performed the following optimizations, updating the original commit (for the reasons mentioned above), which has resulted in a 18x overall size reduction (125 MB to 7 MB; 47 MB to 1.0 MB for the largest GIF—a difference of nearly 50x) with only a marginal loss in perceptible output quality (mostly from the frame rate):

  • Converting them from full 24-bit color to heavily pallettetized 8 bit (I explored lower bit depths, but they non-trivially impacted quality)
  • Downcoverting them to 5 frames per second, from 30, which is mostly unnecessary given the lack of motion (though an argument could be made for 10)
  • Downscaling them appropriately for their final display, down to a target size of approximately 640 px
  • Speeding them up from realtime where appropriate, if the extra speed did not impair following them
  • Cropping out portions that are not relevant to their focus (though they were generally cropped quite well already)
  • Trimming out any parts at the beginning and end that weren't related to their purpose (though again, this was mostly done well already)
  • Optimizing them with gifsicle, combining very similar colors so they compress much better at minimal quality loss
  • Using a variety of other smaller optimizations, including pallette generation and diff calculation mode, disabling dithering, using transparency, stripping extensions, and other ffmpeg and gifsicle tweaks

In case you're interested for the future, I used FFMPEG + gifsicle, with the following invocation that you can easily plug parameters (as shell variables) into:

TEMP_FILENAME="temp_processed"; ffmpeg ${START:+-ss 00:00:}${START:-} ${END:+-t 00:00:}${END:-} -i ${FILENAME}.gif -vf "${CROP:+crop=}${CROP:-}${CROP:+,}setpts=${SPEED:-1}*PTS,fps=5,${SCALE:+scale=}${SCALE:-}${SCALE:+:flags=lanczos,}${PAUSE:+tpad=stop_mode=clone:stop_duration=}${PAUSE:-}${PAUSE:+,}split[s0][s1];[s0]palettegen=stats_mode=diff[p];[s1][p]paletteuse=diff_mode=rectangle:dither=none" -loop 0 -y ${TEMP_FILENAME}.gif && gifsicle --no-conserve-memory -O3 --lossy ${TEMP_FILENAME}.gif > ${FILENAME}.gif && rm ${TEMP_FILENAME}.gif

The parameters I used for each one were:

FILENAME="editor-automatic-formatting"; CROP=; SCALE="458:-1"; START=; END=; SPEED=0.5; PAUSE=2;
FILENAME="editor-class-function-selector"; CROP="736:ih-2:0:2"; SCALE=; START=01; END=; SPEED=0.5; PAUSE=2;
FILENAME="editor-file-switcher"; CROP=; SCALE="640:-1"; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-go-to-definition"; CROP="iw:ih-20:0:4"; SCALE=; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-split-panels"; CROP="iw:ih-40:0:40"; SCALE="640:-1"; START=; END=; SPEED=0.5; PAUSE=1;
FILENAME="editor-syntax-highlighting"; CROP="1578-320:910-226:320:226"; SCALE="640:-1"; START=02; END=07; SPEED=1; PAUSE=1;
FILENAME="editor-tabs-sorting"; CROP="1560:880-46:0:46"; SCALE="640:-1"; START=; END=; SPEED=0.75; PAUSE=2;

Additionally, I had many of them hold on the final frame for 1-2s longer (as a stillframe, at no cost to file size), as many of them cut away a bit too quickly for the viewer to fully grasp a significant final state.

@steff456
Copy link
Member

Thanks @CAM-Gerlach, we will just wait for the review of the rest of the team to merge this PR!

Copy link
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here @hyounes4560 @steff456 and @CAM-Gerlach ! It's a huge improvement! Left a simple comment regarding a text role that maybe needs a change on the Run file section but other than that this LGTM 👍

doc/panes/editor.rst Outdated Show resolved Hide resolved
@CAM-Gerlach CAM-Gerlach requested a review from dalthviz November 15, 2022 01:07
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 15, 2022

I will re-generate the GIFs using the script above and latest original source videos as soon as I get confirmation of those, to improve the clarity and quality.

In the meantime, @steff456 would you like to formally approve this as well? 🙂

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @hyounes4560 for your impressive work on this PR! I left some small suggestions, but overall looks pretty good to me.

Also, thanks to @steff456 and @CAM-Gerlach for your help during this process.

doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
doc/panes/editor.rst Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Nov 15, 2022

Since the changes @ccordoba12 requested were all quite small and straightforward, aside from one that was a theme change, I went ahead and took care of applying them to expedite final merging of this PR.

As discussed previously, I regenerated the GIFs from their source videos; while the quality improvements due to that were rather minimal, as apparently there was still a significant quality issue with the original videos (which seemed likely due to scaling/rescaling, either on capture or during its processing), it did substantially reduce the final file size even further, and I also increased the output resolution and tweaked the scaling factors to extract as much quality from the originals as practicable, which resulted in modest clarity improvements and a further 28%/1.4x reduction in total file size, for a total savings decrease of 24x vs. the original. Parameters are below, for reference:

Conversion parameters
FILENAME="editor-automatic-formatting"; CROP=; SCALE="720:-1"; START=; END=; SPEED=0.5; PAUSE=2;
FILENAME="editor-class-function-selector"; CROP="736:ih-2:0:2"; SCALE=; START=01; END=; SPEED=0.5; PAUSE=2;
FILENAME="editor-file-switcher"; CROP=; SCALE="720:-1"; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-go-to-definition"; CROP="iw:ih-20:0:4"; SCALE=; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-go-to-line"; CROP="iw:ih-32:0:32"; SCALE="720:-1"; START=; END=; SPEED=0.75; PAUSE=2;
FILENAME="editor-split-panels"; CROP="iw:ih-40:0:40"; SCALE="720:-1"; START=; END=; SPEED=0.5; PAUSE=1;
FILENAME="editor-syntax-highlighting"; CROP="1578-320:910-226:320:226"; SCALE="720:-1"; START=02; END=07; SPEED=1; PAUSE=1;
FILENAME="editor-tabs-sorting"; CROP="1440:880-46:0:46"; SCALE="720:-1"; START=; END=; SPEED=0.75; PAUSE=2;

Finally, I spotted and fixed a few final trivial textual polish issues, so this should be now all finally ready to merge.

@CAM-Gerlach
Copy link
Member

Since myself and @dalthviz have approved, and @steff456 and @ccordoba12 's final suggestions were all applied and they both indicated informally that they approved merging this, I'll go ahead with the merge now, so this doesn't have to wait any longer to go live and start benefiting Spyder's users, who I'm sure will be very happy to see the fruits of your tireless efforts here.

Thanks so, so much to @hyounes4560 for all her amazing hard work on this PR and her patience, kindness and understanding in the process, even when it wasn't nearly as smooth as it could have been due to my mistakes and being very busy on other things. Thanks to @steff456 for managing this project, guiding and mentoring Hanan, and contributing a lot to the final product. And finally, thanks to @dalthviz and @ccordoba12 for reviewing this and providing great feedback.

Cheers!

@CAM-Gerlach CAM-Gerlach merged commit f8f6a9b into spyder-ide:master Nov 15, 2022
@CAM-Gerlach
Copy link
Member

And its live!

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.

Add FAQ on how to do complex regex searches with the Find panel Mention how to run cells
5 participants