hackerb9 / lsix

Like "ls", but for images. Shows thumbnails in terminal using sixel graphics.
GNU General Public License v3.0
4.03k stars 127 forks source link

Handle files with leading at signs, update README #50

Open dankamongmen opened 3 years ago

dankamongmen commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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".