search

spyder-ide / spyder-docs

Documentation for Spyder, the Scientific Python Development Environment
https://docs.spyder-ide.org
MIT License
33 stars 283 forks source link

Overhaul Editor doc with many new sections and screenshots #331

Closed hyounes4560 closed 1 year ago

hyounes4560 commented 2 years ago

Pull Request

Pull Request Checklist

Description of Changes

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

CAM-Gerlach commented 1 year ago

Just to clarify @hyounes4560 , assuming you want to use my fixed branch (at least as a starting point), you don't need to actually apply/make any of @steff456 's or my suggested changes here yourself, as I've already applied them there (with author credit for @steff456 , of course) as they would otherwise get lost in the git shenanigans and/or block/conflict with other changes.

steff456 commented 1 year ago

@CAM-Gerlach can you please push from your branch to this one, I think that will make the review process easier and then we can check what's missing from there.

CAM-Gerlach commented 1 year ago

Sure @steff456 ; I considered offering that originally but to fix the Git problem it needed to be a force push, which requires a few extra steps on @hyounes4560 's end to pick up the changes, and I was particularly concerned that if she had existing local changes, or the branch wasn't pulled before making such, they could either be lost or we would be back to square one with the same Git issues that caused the problem in the first place.

At least from my end, this should be nearly ready to go, and I applied all your changes as well. The only thing still missing is the remaining GIFs and images— @hyounes4560 are you still looking for some suggestions on the recently added sections?

In any case, @hyounes4560 here's what you need to do to make sure you pick up these changes locally without losing any work or running into more Git issues. If you need any help, please reach out here, over Slack, over Zoom, etc. and I'll be happy to walk you through it:

  1. Save a backup of your existing branch (I also have one on my fork if you need it), and if you have local uncomitted changes you want to keep, stash them:
git branch --copy HY-wip-update-editor-docs-backup
 # If you have uncommitted changes:
git stash push -m "Editor changes before force-pulling CAM's branch"
  1. Fetch and reset to the updated remote branch on a new branch:
git switch HY-wip-update-editor-docs-new
git fetch origin
git reset --hard origin/HY-wip-update-editor-docs
git switch HY-wip-update-editor-docs
  1. If you have any new commits on your old branch locally that aren't pushed here, cherry-pick or rebase them on top of the new one, and if you have any stashed uncommitted changes, apply them. If so, you might get some merge conflicts you'll need to resolve (but I'm assuming it should just be adding a few images, so it shouldn't be too bad)
# If you have made new commits on your local HY-wip-update-editor-docs after f8f57a94f, the last commit here:
git rebase --onto HY-wip-update-editor-docs-new f8f57a94f HY-wip-update-editor-docs
# If you had any uncommitted changes that you stashed:
git stash apply

You're done! Now you can continue to make changes, commit and push as normal. Again, let me know here, on Slack, etc. if I can help further. Thanks, and looking forward to seeing your all great work merged soon!

steff456 commented 1 year ago

@CAM-Gerlach, so now the comments that @hyounes4560 needs to address are the discussions that are not resolved in the last review?

CAM-Gerlach commented 1 year ago

Actually, as for the two outstanding suggestions from your review, at least from my side they are both resolved (I added a paragraph break in the paragraph you highlighted for the first one, and my personal opinion was there wasn't a need to add the screenshot you asked about in the second one), but I left them unresolved in case you or @hyounes4560 had different suggestions/viewpoints on that.

Aside from that, the only other remaining items are adding the rest of the screenshots and GIFs (the ones that have placeholders in the doc, and possibly some others in the new sections, which I can help suggest if desired), and one small comment I had about the GIF (which otherwise looked very good), which I'll make now.

hyounes4560 commented 1 year ago

Sure @steff456 ; I considered offering that originally but to fix the Git problem it needed to be a force push, which requires a few extra steps on @hyounes4560 's end to pick up the changes, and I was particularly concerned that if she had existing local changes, or the branch wasn't pulled before making such, they could either be lost or we would be back to square one with the same Git issues that caused the problem in the first place.

At least from my end, this should be nearly ready to go, and I applied all your changes as well. The only thing still missing is the remaining GIFs and images— @hyounes4560 are you still looking for some suggestions on the recently added sections?

In any case, @hyounes4560 here's what you need to do to make sure you pick up these changes locally without losing any work or running into more Git issues. If you need any help, please reach out here, over Slack, over Zoom, etc. and I'll be happy to walk you through it:

  1. Save a backup of your existing branch (I also have one on my fork if you need it), and if you have local uncomitted changes you want to keep, stash them:
git branch --copy HY-wip-update-editor-docs-backup
 # If you have uncommitted changes:
git stash push -m "Editor changes before force-pulling CAM's branch"
  1. Fetch and reset to the updated remote branch on a new branch:
git switch HY-wip-update-editor-docs-new
git fetch origin
git reset --hard origin/HY-wip-update-editor-docs
git switch HY-wip-update-editor-docs
  1. If you have any new commits on your old branch locally that aren't pushed here, cherry-pick or rebase them on top of the new one, and if you have any stashed uncommitted changes, apply them. If so, you might get some merge conflicts you'll need to resolve (but I'm assuming it should just be adding a few images, so it shouldn't be too bad)
# If you have made new commits on your local HY-wip-update-editor-docs after f8f57a94f, the last commit here:
git rebase --onto HY-wip-update-editor-docs-new f8f57a94f HY-wip-update-editor-docs
# If you had any uncommitted changes that you stashed:
git stash apply

You're done! Now you can continue to make changes, commit and push as normal. Again, let me know here, on Slack, etc. if I can help further. Thanks, and looking forward to seeing your all great work merged soon!

@CAM-Gerlach Unfortunately, I still have conflicts. If okay could you please fix the issue from your end? alternatively, I can create a new branch and resubmit everything

CAM-Gerlach commented 1 year ago

@hyounes4560 Sure, I can fix the conflicts for you, but I just need to have access to the new changes on your end so I can do so. Could you please do the following to push your local changes so I can see them?

git merge --abort && git rebase --abort  # Abort any ongoing rebase or merge if you have one in progress
git switch HY-wip-update-editor-docs-backup
git stash apply && git commit -a -m "Temp commit of stashed changes"  # If you stashed any uncomitted changes in step 1
git push -u origin HY-wip-update-editor-docs-backup

If you still have trouble with that, I can hop on Slack or a quick call and help you.

Once you let me know you've done so, I'll go ahead, rebase them on this branch, and then you can force-reset to that and have everything synced up with no conflicts. Thanks!

hyounes4560 commented 1 year ago

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

hyounes4560 commented 1 year ago

@hyounes4560 Sure, I can fix the conflicts for you, but I just need to have access to the new changes on your end so I can do so. Could you please do the following to push your local changes so I can see them?

git merge --abort && git rebase --abort  # Abort any ongoing rebase or merge if you have one in progress
git switch HY-wip-update-editor-docs-backup
git stash apply && git commit -a -m "Temp commit of stashed changes"  # If you stashed any uncomitted changes in step 1
git push -u origin HY-wip-update-editor-docs-backup

If you still have trouble with that, I can hop on Slack or a quick call and help you.

Once you let me know you've done so, I'll go ahead, rebase them on this branch, and then you can force-reset to that and have everything synced up with no conflicts. Thanks!

Done this @CAM-Gerlach

CAM-Gerlach commented 1 year ago

@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

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

hyounes4560 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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.

steff456 commented 1 year ago

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

CAM-Gerlach commented 1 year ago

:warning: Important :warning:

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 :slightly_smiling_face: (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: ```shell 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: ```shell 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 commented 1 year ago

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

CAM-Gerlach commented 1 year ago

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? :slightly_smiling_face:

CAM-Gerlach commented 1 year ago

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 ```shell 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 commented 1 year ago

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 commented 1 year ago

And its live!