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

browse: Show symbolic links and target's type properly #1667

Merged
merged 5 commits into from Jun 1, 2017

Conversation

@cez81
Copy link
Collaborator

commented May 12, 2017

1. What does this change do, exactly?

Symbolic links pointing to a directory will show as directory instead if file.

2. Please link to the relevant issues.

#1660

3. Which documentation changes (if any) need to be made because of this PR?

https://caddyserver.com/docs/browse
Update examples and line numbers for "this struct type"-link

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later

I need to add some tests but I'll start with this to see if I'm on the right track :)

@cez81 cez81 force-pushed the cez81:browse_sym_link branch from 3a06180 to d1a0029 May 12, 2017

Browse: Show symbolic links and targets type properly
 * gofmt

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>

@cez81 cez81 force-pushed the cez81:browse_sym_link branch from d1a0029 to 9d320e1 May 12, 2017

@cez81 cez81 changed the title Browse: Show symbolic links and targets type properly Browse: Show symbolic links and target's type properly May 12, 2017

@mholt
Copy link
Member

left a comment

Thanks for starting this, @cez81! It looks like this also handles the case where the symbolic link is to a file.

Does it matter whether it's a symbolic link (i.e. do we need to show that in the UI)?

@@ -426,7 +426,9 @@ footer {
<span class="name">{{html .Name}}</span>
</a>
</td>
{{- if .IsDir}}
{{- if .IsSymlink }}
<td data-order="-1">symbolic link</td>

This comment has been minimized.

Copy link
@mholt

mholt May 12, 2017

Member

Instead of showing the words "symbolic link" under the "Size" column, how about we introduce new icons for symbolic link to file and symbolic link to directory?

This comment has been minimized.

Copy link
@cez81

cez81 May 12, 2017

Author Collaborator

Yes, that was my initial thought but graphics is not my thing :)

This comment has been minimized.

Copy link
@mholt

mholt May 13, 2017

Member

I can update the graphics. How about we take this out (and leave the template unchanged) for now and I will see about getting icons with little arrows or something to indicate a symbolic link?

This comment has been minimized.

Copy link
@cez81

cez81 May 13, 2017

Author Collaborator

Sounds good to me!

URL: url.String(),
ModTime: f.ModTime().UTC(),
Mode: f.Mode(),
IsDir: f.IsDir() || (isSymlink(f) && isSymlinkTargetDir(f)),

This comment has been minimized.

Copy link
@mholt

mholt May 12, 2017

Member

We could move the check for isSymlink() into isSymlinkTargetDir().

This comment has been minimized.

Copy link
@cez81

cez81 May 12, 2017

Author Collaborator

👍

Move symbolic link check in to isSymlinkTargetDir
Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>

@cez81 cez81 force-pushed the cez81:browse_sym_link branch from b2efe6a to baa31ac May 12, 2017

@mholt
Copy link
Member

left a comment

Okay, well, I think this is a step in the right direction. I've been meaning to get new icons for this template anyway, I'll just add a couple icons for symlinks to folders and files. So no need to change the template for now. I'll ping you when I have some new icons ready.

@@ -426,7 +426,9 @@ footer {
<span class="name">{{html .Name}}</span>
</a>
</td>
{{- if .IsDir}}
{{- if .IsSymlink }}
<td data-order="-1">symbolic link</td>

This comment has been minimized.

Copy link
@mholt

mholt May 13, 2017

Member

I can update the graphics. How about we take this out (and leave the template unchanged) for now and I will see about getting icons with little arrows or something to indicate a symbolic link?

@mholt mholt modified the milestone: 0.10.3 May 14, 2017

@mholt

This comment has been minimized.

Copy link
Member

commented May 18, 2017

I'm waiting on this until I have a chance to update the icons, at which point I'll update your branch. Maybe this weekend?

@mholt mholt modified the milestones: 0.10.3, 0.10.4 May 28, 2017

@mholt

This comment has been minimized.

Copy link
Member

commented Jun 1, 2017

I have revised icons for the default template; I will get those pushed later tonight or tomorrow.

They look good I think. :)

@mholt mholt changed the title Browse: Show symbolic links and target's type properly browse: Show symbolic links and target's type properly Jun 1, 2017

mholt added 2 commits Jun 1, 2017
@mholt
mholt approved these changes Jun 1, 2017

@mholt mholt merged commit 132f2a9 into caddyserver:master Jun 1, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@cez81

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 1, 2017

Looks good :)

@cez81 cez81 deleted the cez81:browse_sym_link branch Jun 1, 2017

@mholt

This comment has been minimized.

Copy link
Member

commented Jun 1, 2017

Just noting: a little bug snuck in, related to checking if the file is a symlink. os.Readlink(f.Name()) assumes that f is in the current directory. I'll work on a fix soon...

mholt added a commit that referenced this pull request Jun 2, 2017
reds pushed a commit to reds/caddy that referenced this pull request Jun 8, 2017
browse: Show symbolic links and target's type properly (caddyserver#1667
)

* Browse: Show symbolic links and targets type properly
 * gofmt

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>

* Move symbolic link check in to isSymlinkTargetDir

Signed-off-by: Jonas Östanbäck <jonas.ostanback@gmail.com>

* Revert template change and show sym link folders as normal folders

* browse: Updated icons including symlink indicators
reds pushed a commit to reds/caddy that referenced this pull request Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.