Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[submodule "LibLaserCut"]
path = LibLaserCut
url = https://github.com/t-oster/LibLaserCut.git
url = https://github.com/fablabnbg/LibLaserCut.git
2 changes: 1 addition & 1 deletion LibLaserCut
30 changes: 23 additions & 7 deletions src/main/java/de/thomas_oster/visicut/VisicutModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ public class VisicutModel extends Component // FIXME: "extends Component" isn't

private PlfPart selectedPart = null;
public static final String PROP_SELECTEDPART = "selectedPart";
public static final float ZERO_TOLERANCE = 0.0001f;

private PlfFile plfFile = new PlfFile();

Expand Down Expand Up @@ -866,16 +867,30 @@ public Modification fitObjectsIntoBed()

for(PlfPart p : this.plfFile)
{

boolean modified = false;
Rectangle2D bb = p.getGraphicObjects().getBoundingBox();

result.oldHeight = bb.getHeight();
result.oldWidth = bb.getWidth();



AffineTransform trans = p.getGraphicObjects().getTransform();
//first try moving to origin, if not in range

// automatically move if there are floating point errors
if (bb.getX() < 0 && bb.getX() > -ZERO_TOLERANCE)
{
// adding ZERO_TOLERANCE to show 0 instead of -0 mm as reference point x/y
trans.preConcatenate(AffineTransform.getTranslateInstance(-bb.getX() + ZERO_TOLERANCE, 0));
p.getGraphicObjects().setTransform(trans);
bb = p.getGraphicObjects().getBoundingBox();
}
if (bb.getY() < 0 && bb.getY() > -ZERO_TOLERANCE)
{
trans.preConcatenate(AffineTransform.getTranslateInstance(0, -bb.getY() + ZERO_TOLERANCE));
p.getGraphicObjects().setTransform(trans);
bb = p.getGraphicObjects().getBoundingBox();
}

// if outside of bed, try moving to origin
if (bb.getX() < 0 || bb.getX() + bb.getWidth() > bw)
{
trans.preConcatenate(AffineTransform.getTranslateInstance(-bb.getX(), 0));
Expand All @@ -886,7 +901,7 @@ public Modification fitObjectsIntoBed()
result.newHeight = bb.getHeight();
result.newWidth = bb.getWidth();
result.newX = bb.getX();
result.newY = bb.getY();
result.newY = bb.getY();
}
if (bb.getY() < 0 || bb.getY() + bb.getHeight() > bh)
{
Expand All @@ -898,8 +913,9 @@ public Modification fitObjectsIntoBed()
result.newHeight = bb.getHeight();
result.newWidth = bb.getWidth();
result.newX = bb.getX();
result.newY = bb.getY();
result.newY = bb.getY();
}

//if still too big (we're in origin now) check if rotation is useful
if (bb.getX() + bb.getWidth() > bw || bb.getY() + bb.getHeight() > bh)
{
Expand Down
2 changes: 1 addition & 1 deletion tools/inkscape_extension/visicut_export.inx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<effect needs-live-preview="false">
<object-type>path</object-type>
<effects-menu>
<submenu _name="Lasercut Path"/>
<submenu _name="VisiCut"/>
</effects-menu>
</effect>
<script>
Expand Down
34 changes: 25 additions & 9 deletions tools/inkscape_extension/visicut_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import random
import string
import socket
from pathlib import Path

try:
from os import fsencode
Expand Down Expand Up @@ -140,13 +141,17 @@ def is_exe(fpath):

def inkscape_version():
"""Return Inkscape version number as float, e.g. version "0.92.4" --> return: float 0.92"""
version = subprocess.check_output([INKSCAPEBIN, "--version"], stderr=DEVNULL).decode('ASCII', 'ignore')
assert version.startswith("Inkscape ")
match = re.match(r"Inkscape ([0-9]+\.[0-9]+).*", version)
assert match is not None
version_raw = subprocess.check_output([INKSCAPEBIN, "--version"], stderr=DEVNULL).decode('ASCII', 'ignore')
## When inkscape lives in an appimage, AppRun may pollute stdout with extra information.
# Go through all the lines, and find the one that starts with Inkscape
lines = version_raw.splitlines()
version = [line for line in lines if line.startswith("Inkscape ")]
assert len(version) == 1, "inkscape --version did not return a version number: " + version_raw
match = re.match(r"Inkscape ([0-9]+\.[0-9]+).*", version[0])
assert match is not None, "failed to parse version number from " + version[0]
version_float = float(match.group(1))
return version_float



# Strip SVG to only contain selected elements, convert objects to paths, unlink clones
Expand All @@ -157,7 +162,7 @@ def inkscape_version():
# The idea is similar to http://bazaar.launchpad.net/~nikitakit/inkscape/svg2sif/view/head:/share/extensions/synfig_prepare.py#L181 , but more primitive - there is no need for more complicated preprocessing here
def stripSVG_inkscape(src, dest, elements):
version = inkscape_version()

# create temporary file for opening with inkscape.
# delete this file later so that it will disappear from the "recently opened" list.
tmpfile = tempfile.NamedTemporaryFile(delete=False, prefix='temp-visicut-', suffix='.svg')
Expand Down Expand Up @@ -199,8 +204,8 @@ def stripSVG_inkscape(src, dest, elements):
verbs += ["UnhideAllInAllLayers", "EditInvertInAllLayers", "EditDelete", "EditSelectAllInAllLayers", "EditUnlinkClone", "ObjectToPath", "FileSave"]
# --verb=action1;action2;...
command += ["--verb=" + ";".join(verbs)]


DEBUG = False
if DEBUG:
# Inkscape sometimes silently ignores wrong verbs, so we need to double-check that everything's right
Expand Down Expand Up @@ -248,7 +253,7 @@ def stripSVG_inkscape(src, dest, elements):
actions += ["export-area-page"]

command = [INKSCAPEBIN, tmpfile, "--export-overwrite", "--actions=" + ";".join(actions)]

try:
#sys.stderr.write(" ".join(command))
# run inkscape, buffer output
Expand Down Expand Up @@ -326,6 +331,16 @@ def get_original_filename(filename):
VISICUTBIN = which("VisiCut.Linux", [VISICUTDIR, "/usr/share/visicut"])
INKSCAPEBIN = which("inkscape", [INKSCAPEDIR])

## Test if this inkscape is in an appimage.
# We detect this by checking for an AppRun file, in one of the parent folders of our INKSCAPEBIN.
# If so, replace INKSCAPEBIN with AppRun, as this is the only safe way to call inkscape.
# (a direct call mixes libraries from the host system with the appimage, may or may not work.)
for parent in Path(INKSCAPEBIN).parents:
apprun = parent / "AppRun"
if apprun.is_file() and os.access(apprun, os.X_OK):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this os.access check, it's very compact. pathlib should have a shortcut for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe only checking is_file() alone is even more compact? An inkscape AppImage that has an 'AppRun' file which has no execute bits, or is mounted without exec, or suffering from a security policy, or whatever ... is out of scope here anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just failing silently is not the best idea. Your algorithm can still cross mountpoints (that's what the is_mount check was there for before) and I can imagine a bunch of false positive detections of AppRuns in that case. For instance, in some occasions, AppDirs can be integrated in each other (lazy approach but it works, I use that myself occasionally). In such a case, the execute bit on some AppRun in the chain might not be set. Also, AppImages can be extracted and copied to devices that do not have the executable bit set, etc.

The check is fine and it's good to have it. In fact, security wise, we should add the mountpoint check and abort once it's crossed. To do so, this should be appended to the for's body:

    if parent.is_mount():
        break

Security (i.e., preventing critical issues) and safety (i.e., keeping users from experiencing weird behavior) always add a bit of verbosity to the code, but in the end, it's always worth it. We'd have some good reasons to throw exceptions here even. But that'd bloat this PR. I think in its current form, it's a reasonably solid implementation and I don't see any attack vector (without thinking about it for more than 5 minutes).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I found an INKSCAPE_COMMAND environment variable that seems to point to the AppRun file. Can you check if we can use this instead? First guess:

INKSCAPEBIN = os.environ.get("INKSCAPE_COMMAND") or which("inkscape", [INKSCAPEDIR])

This would avoid the potential problems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jnweiger Could you please change to use INKSCAPE_COMMAND (and check if it works), so that we can merge?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ping :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I no longer remember this issue. Sorry. What should we do?
First check if INKSCAPE_COMMAND exists, if not try INKSCAPE_BIN, if not try to find AppRun with one of the loops discussed here?
Or just use INKSCAPE_COMMAND and fail if it fails?

I cannot comment on issues caused by "AppDirs integrated in each other" -- if an is_mount() check helps, then please add that where appropriate.

I am happy to test drive a PR that incorporates all these wishes and concerns that we collected here.
But writing that PR is beyond me. Sorry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion would be to:

  • use INKSCAPE_COMMAND if available (should definitely work for Inkscape >= 1.3.x, even in AppImage)
  • else determine INKSCAPEBIN from PATH as we did before (maybe needed for very very old Inkscape versions)
  • do not implement anything special for AppImage (because they set INKSCAPE_COMMAND).

So basically replace all code that tries to determine the inkscape path with:

INKSCAPEBIN = os.environ.get("INKSCAPE_COMMAND") or which("inkscape", [INKSCAPEDIR])

If that doesn't work we can still add more complexity.

INKSCAPEBIN = apprun
break

tmpdir = tempfile.mkdtemp(prefix='temp-visicut-')
dest_filename = os.path.join(tmpdir, get_original_filename(filename))

Expand Down Expand Up @@ -384,3 +399,4 @@ def get_original_filename(filename):
sys.exit(1)

# TODO (complicated, probably WONTFIX): cleanup temporary directories -- this is really difficult because we need to make sure that visicut no longer needs the file, even for reloading!
# - Maybe add the PID od the running visicut, then we can detect orphaned temp direcories.
2 changes: 1 addition & 1 deletion tools/inkscape_extension/visicut_export_replace.inx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<effect needs-live-preview="false">
<object-type>path</object-type>
<effects-menu>
<submenu _name="Lasercut Path"/>
<submenu _name="VisiCut"/>
</effects-menu>
</effect>
<script>
Expand Down