-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Improve Files section #364
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for the long delay in reviewing this, given my cat, the trip and my friends and family visiting, it fell through the cracks of my GH notifications and task list. Please feel free to bug me on Discord and/or remind me at meetings (though I'll try to do a better job at keeping up going forward).
Unfortunately, it seems this wasn't based on the upstream master
branch and instead has a bunch of commits from your previous PR, which you can see in the commit history and in the Files changed tab. However, a minor bit of Git surgery will resolve this; either I can take care of it for you, or you can run the following commands:
git switch update_doc
git reset --hard master
git cherry-pick c640ca9efc2c6aa3b4236b81d37edac6b7fc8fb2
git push --force --set-upstream origin update_doc
(The --set-upstream origin update_doc
is included in case you haven't already set this as your upstream tracking branch)
I also have a couple comments on the intended changes themselves, but I'll hold them till after fixed to avoid any hiccups resolving this issue. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our discussion, I went ahead with implementing the "Git surgery" to fix the branch history issue identified. If you want to work on this branch further locally (instead of just on GitHub via creating or accepting suggestions), you'll need to run the following in your repo before working on it to pull down the updated version (since I had to inherently rewrite Git history to fix the problem):
git switch update_doc
git fetch --all
git reset --hard origin/update_doc
Besides that, as mentioned I did have some feedback on one of the changes, which I made as a review comment. Thanks!
@@ -81,7 +81,7 @@ File associations | |||
================= | |||
|
|||
:guilabel:`Files` allows you to associate different external applications with specific file extensions they can open. | |||
Under the :guilabel:`File associations` tab of the :guilabel:`Files` preferences pane, you can add file types and set the external program used to open each of them by default. | |||
Under the :guilabel:`Preferences --> File associations` tab of the :guilabel:`Files` preferences pane, you can add file types and set the external program used to open each of them by default. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear on the motivation for this change was made, since it implies two conflicting potential interpretations, neither of which is correct (unlike the previous text):
- As mentioned, the existing text already states that this is the
File associations
tab of theFiles
preferences pane, so its confusing why "Preferences" is repeated again here - On first reading, this change implies that
File associations
is a pane underneath preferences, but that's not the case as mentioned - Furthermore, in context as written, it alternatively implies there is a
Preferences
in theFiles
preferences pane (and underneath that aFile associations
tab), but that is of course not correct either - Additionally,
:guilabel:
is for single, specific GUI labels; to support multiple levels of things you'd need:menuselection:
Without more context on the desired intent here, I can propose either a revert to the existing, correct text which explicitly specifies that this is a tab of a specific pane under preferences:
Under the :guilabel:`Preferences --> File associations` tab of the :guilabel:`Files` preferences pane, you can add file types and set the external program used to open each of them by default. | |
Under the :guilabel:`File associations` tab of the :guilabel:`Files` preferences pane, you can add file types and set the external program used to open each of them by default. |
Or, we could specify the complete path all in one go, which is more direct and concise but lacks explicit cues for the reader as to what they should be looking for at each level, and also runs into the issue you raised in #365 which will take some engineering work to solve, and is de-facto gated by the implementation of the new theme, so I don't recommend this for now:
Under the :guilabel:`Preferences --> File associations` tab of the :guilabel:`Files` preferences pane, you can add file types and set the external program used to open each of them by default. | |
Under :menuselection:`Preferences --> Files --> File associations`, you can add file types and set the external program used to open each of them by default. |
If you could help me understand better the problem this is intended to solve, I could try to help propose a better solution. Thanks!
This PR includes updates to improve clarity and accuracy in the documentation for the Files section: