-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
8285306: Fix typos in java.desktop #8328
Conversation
👋 Welcome back ihse! A progress list of the required criteria for merging this PR into |
Webrevs
|
change -> changed src/java.desktop/macosx/classes/sun/lwawt/macosx/CEmbeddedFrame.java
invoced -> invoked src/java.desktop/macosx/native/libjsound/PLATFORM_API_MacOSX_MidiOut.c
"is the" src/java.desktop/windows/native/libjsound/PLATFORM_API_WinOS_MidiOut.c
same as above Regarding changes in gif + freetype diff --git a/src/java.desktop/share/native/libfontmanager/freetypeScaler.c b/src/java.desktop/share/native/libfontmanager/freetypeScaler.c Please exclude ALL 3rd party libraries from this PR. |
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.
Nearly 500 files are too many. Smaller chunks would be easier to review.
Some of the native code files could come from upstream libraries.
src/java.desktop/macosx/classes/com/apple/laf/AquaButtonUI.java
Outdated
Show resolved
Hide resolved
src/java.desktop/macosx/classes/sun/lwawt/macosx/CEmbeddedFrame.java
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/native/libawt/windows/awt_PrintDialog.cpp
Outdated
Show resolved
Hide resolved
src/java.desktop/windows/native/libawt/windows/awt_PrintJob.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
…ata.m Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
…aphicsConfig.m Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
…rtexCache.m Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
…rtexCache.m Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
….cpp Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
…eListener.java Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
/contributor add @turbanoff |
@magicus |
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.
Is src/java.desktop/share/native/libawt/awt/image/gif/gifdecoder.c
a third-party code?
J2dTraceLn4(J2D_TRACE_VERBOSE, "MTLLayer.blitTexture: uninitialized (mtlc=%p, javaLayer=%p, buffer=%p, device=%p)", self.ctx, self.javaLayer, self.buffer, ctx.device); | ||
J2dTraceLn4(J2D_TRACE_VERBOSE, | ||
"MTLLayer.blitTexture: uninitialized (mtlc=%p, javaLayer=%p, buffer=%p, device=%p)", self.ctx, | ||
self.javaLayer, self.buffer, ctx.device); |
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.
Is this an intended change? Or does it come from a merge?
May I suggest wrapping the line after the format string before self.ctx
? It would make it easier to read.
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.
This is from the merge. The only difference wrt to current code is "devide" => "device". I can change wrapping if you request it, but I'd prefer to only fix typos.
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'd prefer to only fix typos.
Let us keep only the typos.
I don't think so. I find no comments in the source code, or any kind of indication on this being so. If you say it is, I can revert the changes in that file. |
I see now that Phil cryptically said:
That might be interpreted as stating that |
I don't know why I mentioned those two files like that but those particular two are JDK code so are fair game. I did write You did fix the latter, but there are still some 3rd party files in there that are edited : glext.h, wsutils.h, multiVis.c |
@prrace I have now reverted the changes to glext.h, wsutils.h and multiVis.c. Is this finally ready for merging? (Going forward, I think we absolutely need to have some way to document in the code tree that certain files is 3rd party, like the UPSTREAM notation I previously suggested, or some variant thereof. This "tribal knowledge" about what is 3rd party is not beneficial to anyone, and only wastes both your and my time...) |
@@ -65,7 +65,7 @@ | |||
*/ | |||
public abstract class MetalTheme { | |||
|
|||
// Contants identifying the various Fonts that are Theme can support | |||
// Constants identifying the various Fonts that a Theme can support |
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 think this is possibly meant to say "constants that our theme can support"?
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.
Please submit a new JBS issue for it if you can; if you can't, I can submit one on your behalf.
Possibly it meant to say “the theme”.
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 unfortunately don't have a JBS (yet..), so if you could, that would be amazing.
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 think the text is correct here: …that a Theme can support
MetalTheme
is a superclass for classes which implement Themes. Thus the constants define fonts which a particular theme may use.
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.
Makes sense.
@@ -1431,7 +1431,7 @@ public IllegalThreadException() { | |||
private static final int MINIMAL_DELAY = 5; | |||
|
|||
/** | |||
* Parameterless version of realsync which uses default timout (see DEFAUL_WAIT_TIME). | |||
* Parameterless version of realsync which uses default timeout (see DEFAULT_WAIT_TIME). |
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.
* Parameterless version of realsync which uses default timeout (see DEFAULT_WAIT_TIME). | |
* Parameterless version of realsync which uses the {@link #DEFAULT_WAIT_TIME default timeout}. |
I mean this suggestion might not be warranted.
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 for a clearer message:
Parameterless version of {@code realsSync}
which uses the default timeout of {@link #DEFAULT_WAIT_TIME}
.
Let us address this in a separate issue.
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.
JDK-8294255: Add link to DEFAULT_WAIT_TIME in javadoc for SunToolKit.realsSync
Feel free to provide a patch.
Everything looks fine, and you can freely ignore my suggestions. They are just thoughts. And yeah we should probably earmark third party files to avoid "tribal knowledge". |
/integrate |
@magicus This pull request has not yet been marked as ready for integration. |
@prrace Do you think you can approve this now, so we can finally close it? (I promise I won't open huge PRs like this in the future; lesson well learnt.) |
@magicus This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 89 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 2a799e5.
Your commit was automatically rebased without conflicts. |
I ran
codespell
on thesrc/java.desktop
directory, and accepted those changes where it indeed discovered real typos.I ignored typos in public methods and variables. Maybe they can be fixed later on without much fanfare, if they are in internal classes. Typos in exposed APIs are likely here to stay.
I will update copyright years using a script before pushing (otherwise like every second change would be a copyright update, making reviewing much harder).
The long term goal here is to make tooling support for running
codespell
. The trouble with automating this is of course all false positives. But before even trying to solve that issue, all true positives must be fixed. Hence this PR.Progress
Issue
Reviewers
Contributors
<aturbanov@openjdk.org>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/8328/head:pull/8328
$ git checkout pull/8328
Update a local copy of the PR:
$ git checkout pull/8328
$ git pull https://git.openjdk.org/jdk pull/8328/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 8328
View PR using the GUI difftool:
$ git pr show -t 8328
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8328.diff