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

Fix of #522 #523

Merged
merged 9 commits into from
Nov 2, 2016
Merged

Fix of #522 #523

merged 9 commits into from
Nov 2, 2016

Conversation

ra3xdh
Copy link
Contributor

@ra3xdh ra3xdh commented Jun 13, 2016

This PR contains fix for #522 and some refactoring at the ImageWriter class. @in3otd , please check on your side.

ra3xdh added 3 commits June 13, 2016 11:14
This fixes original issue Qucs#522. Diagram markers are taken into account
when calculating selected area width and height.
1. Method getSelAreaWidthAndHeight() moved from Schematic to ImageWriter class;
2. Duplicated code in ImageWriter moved to UpdateMinMax() method
@in3otd
Copy link
Contributor

in3otd commented Jun 17, 2016

thanks, seems to work almost as expected.
If I have a Diagram with Markers and select only the Diagram, when printing the selection the marker lines in the Diagram area are still printed (as before) but the printed area is cropped to fit the Diagram only (as expected). Ideally it should be as for the schematic and wire labels, if the wire labels are not selected the labels lines are not visible.
I guess the issue with Diagrams comes from here - the markers are always painted, even if not selected.

Diagram::paint() was splitted into two methods: Diagram::painDiagram()
and Diagram::paintMarkers(). It allow to print diagram without markers
and supress marker lines when only diagram is selcted and markers are
outside diagram.
@ra3xdh
Copy link
Contributor Author

ra3xdh commented Jun 20, 2016

@in3otd I have divided Diagram::paint() into two methods: Diagram::paintDiagram() and Diagram::paintMarkers() This makes possible to paint diagram and markers independently. Check the last commit. Now markers will be not printed if they are not selected.

@in3otd
Copy link
Contributor

in3otd commented Jul 24, 2016

printing selected object seems to work well now, thanks.
There are still some details which are not fully right when printing the entire document (present also in the current release-0.0.19 code):

  • expand the enclosed project
  • open the schematic
    • do File->Export as image then Export
    • the wire/node labels on the bottom are slightly cut (and there should be some margin anyway) in the exported image
  • open the Data Display (do not move anything inside)
    • do File->Export as image then Export
    • the diagrams are cut on the left and right side in the exported image
    • if you move "something" in the Data Display then it's printed correctly, sometimes

1. Schematic->UsedX2,UsedY2 were not reliable in some cases. Replaced by
sizeOfAll() method.
2. Removed two forgotten commented lines
@ra3xdh
Copy link
Contributor Author

ra3xdh commented Jul 25, 2016

I have corrected image size determination method. Previous one was unreliable in some cases. See the latest commit.

@guitorri guitorri added this to the 0.0.19 milestone Oct 28, 2016
@in3otd
Copy link
Contributor

in3otd commented Oct 31, 2016

thanks, it's quite better, but not yet perfect, sorry...

In the example project linked above

  • open the schematic
  • move one wire label on the top side, above the components
  • select a few components and the label above the components
  • do File->Export as image then Export, check Export selected only
  • the label above the components is cut (happens also if the label on the right or left, but not on the bottom...)

Besides, if printing the entire schematic, labels on the bottom, below any components, as in the example project linked above do not have any margin below them, while the margin is correctly present on the other sides.

@ra3xdh
Copy link
Contributor Author

ra3xdh commented Oct 31, 2016

Current implementation inherits label bounding box determination method from the legacy code Schematic::sizeOfAll() method. It seems it has a bug. I will try to dig into this code.

1. Wire label bounding box is determined correctly. Introduced
WireLabel::getLableBounding() method. It is used for selected labels
and Schematic::sizeOfAll() method.

2. Corrected wire size determination for selected wires.
@ra3xdh
Copy link
Contributor Author

ra3xdh commented Oct 31, 2016

@in3otd , I have modified legacy code. Now label bounding box should be determined correctly. Here are two examples of exported images. The first uses "Export selected" and the second uses "Export All":
export1
export

@in3otd
Copy link
Contributor

in3otd commented Nov 1, 2016

thanks for the quick fix.

While testing I noticed one inconsistency: the bounding box calculations take into account labels only if their associated wire is selected while the painting routine print selected labels even if the associated wire is not selected. The end result is that wire labels can be cut if the corresponding wire is not selected.
It's certainly a corner case, but I'll suggest following what the painting routine does and so include the selected labels in the bounding box calculations unconditionally.

Another details is that if you toggle the Original size checkbox while Export selected only is checked, the Width/Height lines show the full schematic size and not just the selected components area size.

I've pushed a PR to your branch with the small fixes for these things, please take a look.

@ra3xdh
Copy link
Contributor Author

ra3xdh commented Nov 2, 2016

@in3otd , Your code works as expected. If you have no other additions, this PR could be merged after CI will be passed.

@guitorri
Copy link
Member

guitorri commented Nov 2, 2016

Seems good to me as well. Merging.

@guitorri guitorri merged commit b57fb42 into Qucs:release-0.0.19 Nov 2, 2016
@ra3xdh ra3xdh deleted the 522_fix branch November 24, 2016 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants