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

8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS) #3250

Closed

Conversation

@azvegint
Copy link
Member

@azvegint azvegint commented Mar 29, 2021

This fix is to explicitly specify that Taskbar::getIconImage may return an object different from passed to Taskbar::setIconImage.

Actually it is always returns a different object on macOS(the only OS which supports this feature), but I want to save some options if we decide to rework this.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3250/head:pull/3250
$ git checkout pull/3250

Update a local copy of the PR:
$ git checkout pull/3250
$ git pull https://git.openjdk.java.net/jdk pull/3250/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3250

View PR using the GUI difftool:
$ git pr show -t 3250

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3250.diff

@azvegint
Copy link
Member Author

@azvegint azvegint commented Mar 29, 2021

/csr

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 29, 2021

👋 Welcome back azvegint! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the csr label Mar 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 29, 2021

@azvegint has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@azvegint this pull request must refer to an issue in JBS to be able to link it to a CSR request. To refer this pull request to an issue in JBS, please use the /issue command in a comment in this pull request.

Loading

@openjdk openjdk bot added the rfr label Mar 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 29, 2021

@azvegint The following label will be automatically applied to this pull request:

  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the awt label Mar 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Loading

@azvegint azvegint changed the title JDK-8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS) 8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS) Mar 29, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 27, 2021

@azvegint This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@azvegint
Copy link
Member Author

@azvegint azvegint commented Apr 30, 2021

CSR added.

Loading

@prrace
Copy link
Contributor

@prrace prrace commented May 7, 2021

I've read (quickly) the comments in the bug.
I am not settled on what I think is the right answer here, but the spec words in the PR "may not be equal" leave too many
doubts in the mind of a reader as to what not equal means

I can see there are words added in the bug report that haven't yet made it into the PR
"however it should contain at least one visually similar image."

Now we are getting into implicitly saying you might get back a MultiResolutionImage.

Visually similar could be open to interpretation if we have a MIR where the renderings are different as can reasonably happen when a higher resolution allows for a more realistic image ..

Do we definitely not want to just hang on to whatever an application set so we can return it without modification ?
What are the problems with that ?
Leaving aside specification. what does an app really want this to return ?
Should we start from there ?

Loading

@azvegint
Copy link
Member Author

@azvegint azvegint commented May 8, 2021

Leaving aside specification. what does an app really want this to return ?
Should we start from there ?

We are returning current dock icon image from system.

Do we definitely not want to just hang on to whatever an application set so we can return it without modification ?
What are the problems with that ?

It might be a good solution, but I see some difficulties here.
If we just save passed image instance, its content might be modified by user between setIconImage() and getIconImage() calls.

setIconImage() internally converts passed image, so it does not follow its changes.
Thus we will get a discrepancy between result from getIconImage() and actual dock icon image on macOS side.
(Probably we can mitigate this by modifying the javadoc with some vague wording though.)

Ideally we should make a copy of an icon passed to setIconImage() method to prevent modification of its content.
setIconImage()/getIconImage() works with too wide type java.awt.Image, and I am not aware of any reliable way to clone every possible subtype of it.

Loading

@azvegint
Copy link
Member Author

@azvegint azvegint commented May 27, 2021

@mrserb @dbessono please review

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 17, 2021

@azvegint this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout Taskbar_getIconImageSpec_8264125
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Loading

@openjdk openjdk bot removed the merge-conflict label Jun 17, 2021
@@ -322,7 +322,7 @@ public PopupMenu getMenu() {
}

/**
* Changes this application's icon to the provided image.
* Suggests the system to change this application's icon to the provided {@code image}.
Copy link
Contributor

@prrace prrace Jun 17, 2021

Choose a reason for hiding this comment

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

Suggests -> Requests

Loading

@@ -338,6 +338,10 @@ public void setIconImage(final Image image) {

/**
* Obtains an image of this application's icon.
* <p>
Copy link
Contributor

@prrace prrace Jun 17, 2021

Choose a reason for hiding this comment

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

As discussed off-line, preface this with "@APinote"

Loading

Copy link
Member Author

@azvegint azvegint Jun 17, 2021

Choose a reason for hiding this comment

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

updated

Loading

prrace
prrace approved these changes Jun 17, 2021
@openjdk openjdk bot removed the csr label Jun 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 25, 2021

@azvegint 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:

8264125: Specification of Taskbar::getIconImage doesn't mention that the returned image might not be equal to the Taskbar::setIconImage one. (eg on Mac OS)

Reviewed-by: kizune, prr

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 447 new commits pushed to the master branch:

  • 35c4702: 8268967: Update java.security to use switch expressions
  • b565459: 8267138: Stray suffix when starting gtests via GTestWrapper.java
  • 1d16797: 8268469: Update java.time to use switch expressions
  • ffa34ed: 8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"
  • fdcae66: 8269092: Add OldObjectSampleEvent.allocationSize field
  • fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
  • 3a8f3d6: 8269280: (bf) Replace StringBuffer in *Buffer.toString()
  • c37988d: 8268276: Base64 Decoding optimization for x86 using AVX-512
  • 08ee7ae: 8268855: Cleanup name handling in the Thread class and subclasses
  • c79034e: 8269303: Remove unnecessary forward declaration of PSPromotionManager in cpCache.hpp
  • ... and 437 more: https://git.openjdk.java.net/jdk/compare/a4c46e1e4f4f2f05c8002b2af683a390fc46b424...master

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 master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Jun 25, 2021
@azvegint
Copy link
Member Author

@azvegint azvegint commented Jun 26, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 26, 2021

Going to push as commit 51a1299.
Since your change was applied there have been 451 commits pushed to the master branch:

  • 3b83bc1: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance
  • 68ef21d: 4847239: (spec) File.createTempFile() should make it clear that it doesn't create the temporary directory
  • 3fae4b3: 6633375: FileOutputStream_md.c should be merged into FileOutputStream.c
  • 223759f: 8266901: Clarify the method description of Duration.toDaysPart()
  • 35c4702: 8268967: Update java.security to use switch expressions
  • b565459: 8267138: Stray suffix when starting gtests via GTestWrapper.java
  • 1d16797: 8268469: Update java.time to use switch expressions
  • ffa34ed: 8265919: RunThese30M fails "assert((!(((((JfrTraceIdBits::load(value)) & ((1 << 4) << 8)) != 0))))) failed: invariant"
  • fdcae66: 8269092: Add OldObjectSampleEvent.allocationSize field
  • fd43d9c: 8269225: JFR.stop misses the written info when the filename is only specified by JFR.start
  • ... and 441 more: https://git.openjdk.java.net/jdk/compare/a4c46e1e4f4f2f05c8002b2af683a390fc46b424...master

Your commit was automatically rebased without conflicts.

Loading

@openjdk openjdk bot closed this Jun 26, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 26, 2021

@azvegint Pushed as commit 51a1299.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants