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

Reduce "$HOME/foo/baz.png" to "~/foo/baz.png" #253

Closed
wants to merge 1 commit into from

Conversation

0ion9
Copy link
Contributor

@0ion9 0ion9 commented Apr 7, 2022

This is a fairly straightforward change that makes some long absolute paths easier to read at the cost of slightly increased complexity in update_info()

The only related possible change I can think of that might be reasonable to incorporate is, thumbs.c also reads $HOME. Since this PR makes homedir a global variable, thumbs.c could be simplified slightly by just using the global variable instead.

EDIT: actually, neither main.c nor thumbs.c simply 'reads $HOME'. I didn't notice this initially, but both of them read a XDG_* variable, or HOME if that doesn't exist. Which means homedir may be set to either something like /home/me or something like /home/me/.config. That's a little hacky, and I'd like to try to fix it properly in this PR.

Copy link
Member

@N-R-K N-R-K left a comment

Choose a reason for hiding this comment

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

actually, neither main.c nor thumbs.c simply 'reads $HOME'. I didn't notice this initially, but both of them read a XDG_* variable, or HOME if that doesn't exist.

There's a couple other problems with this PR.

else
strncpy(l->buf, files[fileidx].name, l->size);
} else {
const char *filename = files[fileidx].name;
Copy link
Member

Choose a reason for hiding this comment

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

files.name is given by the user, as is. So this won't work if the user gave relative path.

$ cd $HOME
$ nsxiv ../user/pic/a.png

in this case, .name will be ../user/pic/a.png

Copy link
Contributor Author

Choose a reason for hiding this comment

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

..that is intended. I don't see why it would make sense to shorten the absolute path in the case where the user hasn't given an absolute path. That would be confusing IMO.

main.c Show resolved Hide resolved
@N-R-K
Copy link
Member

N-R-K commented Apr 7, 2022

Overall, I'm not too sold on this since this is something that doesn't really provide any functional benefit and is merely visual.

@0ion9
Copy link
Contributor Author

0ion9 commented Apr 7, 2022

.. isn't being able to see the filename in full a functional benefit? I made this patch because i had the font size set largish and couldn't actually see the full basename of the current image.

@N-R-K
Copy link
Member

N-R-K commented Apr 7, 2022

isn't being able to see the filename in full a functional benefit?

Replacing home with ~ doesn't fully solve that problem. Also I think what you're trying to do in this pr can be more easily done in image-info, can't it?

…humbnail mode

Does not implement ~$USER/ substitution (as you may use in your shell -
eg. `sudo ls ~root`), only the substitution $HOME/ -> ~/
@0ion9
Copy link
Contributor Author

0ion9 commented Apr 7, 2022

.. No it cannot be done with image-info, as image-info doesn't apply to thumbnail mode AFAICS. Sorry i wasn't clear that this is for thumbnail mode - the actual commit message does specify that.

Agreed that $HOME abbreviation doesn't fully solve the issue. It was the answer that seemed to be moderately effective while also being uncontroversial. (as opposed to something like ellipsizing the pathname section until it fits in the bar width)

I've pushed an update, not sure yet whether I need to do something here with 'resolve conversation' to indicate it addresses these issues.

@N-R-K
Copy link
Member

N-R-K commented Apr 7, 2022

.. No it cannot be done with image-info, as image-info doesn't apply to thumbnail mode AFAICS.

Ah my bad. But now that makes me realise that if we do it for thumbnails mode, then it's probably going to be expected for this to be done on image mode as well.

I'm personally not too against the idea of drawing ellipsis at the front in case of truncation. But that's going to be messy due to utf8.

Now I'm wondering if it's worth it to just have a thumb-info as discussed here https://github.com/nsxiv/nsxiv/issues/88, which will just receive the image name.

@0ion9
Copy link
Contributor Author

0ion9 commented Apr 7, 2022

thumb-info seems like a nice idea. I would definitely have a use for it personally (sometimes I want the bar to show image-info even in thumbnail mode.)

I guess plain $HOME substitution would be less of a problem in the case that it came to be expected that the shortening was applied in image mode too. As there is no implication of adjusting directly to bar width, a user could just use sed "s,^$HOME/,~/," on the filename provided and get a comparable result.

utf8 ellipsization.. yeah, messy and either additional data or an additional dependency required.

@N-R-K
Copy link
Member

N-R-K commented Apr 10, 2022

I would definitely have a use for it personally (sometimes I want the bar to show image-info even in thumbnail mode.)

We had to abandon the thumb-info idea due to this: https://github.com/nsxiv/nsxiv/issues/88#issuecomment-1029682293

But now I'm remembering that Imlib2 claims to not decode the images straight away (if possible, might not be possible on some image formats), so maybe loading the original image to get the height/width might not be that bad performance wise.

But even if we just provide the image path as a single argument, users can still get the w/h from it via some cli tool if they wish; so I guess it won't be entirely useless.


@XPhyro Any thoughts on the matter?

@XPhyro
Copy link
Member

XPhyro commented Apr 12, 2022

I think this PR should just be left as a patch on nsxiv-extra, or be superseded by thumb-info.


On thumb-info, I don't have a pronounced opinion on whether and how we implement it. I imagine it would be useful, but would also add some unneeded complexity if we strive to provide arguments that need the image to be decoded or the metadata to be parsed.

The single-argument case you mention seems like a good balance between feature and non-complexity. This would also mean the image would not have to be processed twice if the user wants to show more than what we provide.

@N-R-K
Copy link
Member

N-R-K commented Apr 12, 2022

This would also mean the image would not have to be processed twice if the user wants to show more than what we provide.

Good point. There's cli tools for displaying things like exif data which users might want to use. For image-info passing w/h didn't matter since we already had those info.

N-R-K added a commit to N-R-K/nsxiv that referenced this pull request Apr 22, 2022
@N-R-K N-R-K mentioned this pull request Apr 22, 2022
@N-R-K N-R-K closed this in #265 May 3, 2022
N-R-K added a commit that referenced this pull request May 3, 2022
defaultxr pushed a commit to defaultxr/nsxiv that referenced this pull request Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants