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

Handle files with leading at signs, update README #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dankamongmen
Copy link

The bit about empty strings no longer appears to be the case, at least with ImageMagick 6.9.11-60. Update README. When supplied an initial '@', prefix it with './' to work around another IM issue. Update README.

@hackerb9
Copy link
Owner

hackerb9 commented Sep 5, 2021

Are you sure about empty filenames working? I have the same version of ImageMagick and I have to hit ^C to cancel.

$ convert --version
Version: ImageMagick 6.9.11-60 Q16 x86_64 2021-01-25 https://imagemagick.org
$ convert "" foo.png
^C

I do like that you encapsulated imescape in a function, but doesn't it seem like overkill to fork a new sed process for every filename on the off-chance it starts with @? I know, in this day and age where I literally have dozens of cores sitting idle as I type, just hoping for something to do, it probably doesn't matter. But it feels sloppy. Not that I'm opposed to that when it serves some higher function, like improved readability.

Using Bash's builtin regular expressions, or even just globbing, would be one way to avoid a fork, but it still isn't ideal:

imescape() {
  [[ $1 == @* ]] && echo -n ./
  echo "$1"
}

I want to resist the impulse to let lsix accrete with lots of twiddly little fixes for corner cases. I feel like there's got to be a better way, at least for the problem of filenames that begin with @, which I believe should be passingly rare.

@dankamongmen
Copy link
Author

I want to resist the impulse to let lsix accrete with lots of twiddly little fixes for corner cases. I feel like there's got to be a better way, at least for the problem of filenames that begin with @, which I believe should be passingly rare.

I wanted to do the minimum patch, size-wize, to remedy this issue. If you're comfortable with me making the change at a higher level, i'm pretty sure i can combine all of this into one sed, or come up with a better solution. let me play around with imagemagick a bit today and see what can be done.

@hackerb9
Copy link
Owner

hackerb9 commented Sep 5, 2021

I think I missed something. What else were you going to fix with 'sed'?

I'm open to a larger patch if it helps me to understand the grand scheme.

@dankamongmen
Copy link
Author

I think I missed something. What else were you going to fix with 'sed'?

I'm open to a larger patch if it helps me to understand the grand scheme.

oh, well you've just already got several sed invocations in there; i figure i can fold this into one, or do something else.

there's no grand scheme =] just gonna take an hour at some point and see if this can be slimmed down =]

@hackerb9
Copy link
Owner

hackerb9 commented Sep 5, 2021

Ah, you are referring to processlabel(), the seamy underbelly of lsix that features sed | tr | awk | sed. While it might seem a natural place to put your fixes, its purpose is the opposite. It undoes the munging of filenames ImageMagick demands and puts them in a format suitable for labels.

That discreditable bit of code has long bothered me and eventually I would like to remove it. I think the ideal solution would be if ImageMagick's montage understood how to shorten filename labels (by wrapping or eliding) so they do not extend beyond a tile.

Similarly, it may make the most sense to submit a patch to ImageMagick so that it does not get confused by a filename of "" or "@foo".

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.

2 participants