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

Further fix of screen capture #6893

Closed
wants to merge 5 commits into from
Closed

Conversation

Benglin
Copy link
Contributor

@Benglin Benglin commented Jul 15, 2016

Purpose

Fix user reported issue #6834 where preview bubble goes beyond the edge of captured PNG image. This is mainly because the position of NodeView was replaced by the values of Canvas.LeftProperty and Canvas.TopProperty. These values should be used to compute the final offset (i.e. TranslateTransform) of image content prior to rendering it. The fix is to determine the minimum corner of the entire graph contents, and use it to offset the rendered image contents.

Before this fix:

capture-before

After this fix:

capture-after

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.

Reviewers

Hi @ke-yu, please help to take a look at this fix, hopefully I have explained it with enough clarity. Thanks!

FYIs

Thanks for reporting this issue, @vykrum!

Hi @riteshchandawar, if this is tested working on the master branch, I will then cross-merge it to release branch. Thanks!

@Benglin Benglin added the PTAL Please Take A Look 👀 label Jul 15, 2016
@Benglin Benglin mentioned this pull request Jul 15, 2016
case "NodeView":
case "NoteView":
case "AnnotationView":
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

this break is ambiguous in this context.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it ambiguous?

Copy link
Contributor

Choose a reason for hiding this comment

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

they are followed by a default : continue statement, but break is for switch and continue is for for loop. sort of context switch..

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks fairly standard to me.

@Benglin Benglin closed this Aug 16, 2016
@Benglin Benglin deleted the master branch August 16, 2016 03:39
@Benglin Benglin mentioned this pull request Aug 16, 2016
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants