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

Symlink icons (#1416) #3826

Merged
merged 7 commits into from
May 1, 2018
Merged

Symlink icons (#1416) #3826

merged 7 commits into from
May 1, 2018

Conversation

tf198
Copy link
Contributor

@tf198 tf198 commented Apr 21, 2018

Uses the octicon icons for directory and file symlinks.

screenshot-2018-4-21 symlink_test

Directory symlinks will only be rendered if the target is present in the repository. Broken links, targets outside the repo and links to other links will just be rendered as file-symlink-file.

@tf198
Copy link
Contributor Author

tf198 commented Apr 21, 2018

I have some tests to add before this is merged but I need some advice as to the cleanest way to add a repository to the test server (gitea-repositories-meta, fixtures etc).

Is there an easy way to spin up the test server, push a new repository and spit out the fixtures?

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 21, 2018
@codecov-io
Copy link

codecov-io commented Apr 21, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@1928920). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3826   +/-   ##
=========================================
  Coverage          ?   20.21%           
=========================================
  Files             ?      145           
  Lines             ?    29014           
  Branches          ?        0           
=========================================
  Hits              ?     5864           
  Misses            ?    22259           
  Partials          ?      891
Impacted Files Coverage Δ
modules/base/tool.go 68.36% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1928920...25039cd. Read the comment docs.

@lafriks lafriks added the type/enhancement An improvement of existing functionality label Apr 21, 2018
@lafriks lafriks added this to the 1.5.0 milestone Apr 21, 2018
@lafriks
Copy link
Member

lafriks commented Apr 21, 2018

You can add git repositories in: https://github.com/go-gitea/gitea/tree/master/integrations/gitea-repositories-meta and use it later in integration tests

@tf198
Copy link
Contributor Author

tf198 commented Apr 21, 2018

I tried just adding a bare repo at integrations/gitea-repositories-meta/user2/repo4.git but the test instance wont pick it up unless it exists in the database as well...

@lafriks
Copy link
Member

lafriks commented Apr 21, 2018

Yes you have to add needed database records in fixtures yml files

Signed-off-by: Tris Forster <[email protected]>
@lafriks
Copy link
Member

lafriks commented Apr 21, 2018

Have not tested yet but does opening link also go to link target?

@tf198
Copy link
Contributor Author

tf198 commented Apr 21, 2018

No, behaviour is identical github.

  • all entries with mode 120000 render as file-symlink-file
  • if the target resolves to a directory within the repo it renders as file-symlink-directory
  • symlinks open as a text file with the location of the link.
  • the edit button is present but editing disallowed on attempt to save.
  • link -> link -> directory displays as file-symlink-file i.e. no recursive dereferencing.

See https://github.com/tf198/symlink_test

It would be easy to add an goto target button on the file view page for symlinks if that was considered a useful enhancement...

@thehowl
Copy link
Contributor

thehowl commented Apr 22, 2018

Just a note: editing of symlinks is allowed on GitHub, and I think it should be kept this way (there may be a broken symlink because the dst directory changed - should be reasonable to keep the ability to change the symlink)

Please do add the Go to target button! (I'm wondering why GitHub considered it not necessary...) Thank you for your pull request!

@silverwind
Copy link
Member

Thanks for doing this! Suggestions:

  • Color the directory symlink icon blue like regular directories
  • Maybe (if it's not too hard) sort directory symlink among directories

@silverwind silverwind mentioned this pull request Apr 25, 2018
@lunny
Copy link
Member

lunny commented Apr 29, 2018

LGTM

@silverwind
Copy link
Member

Here's a patch for the directory symlink color:

diff --git a/public/less/_repository.less b/public/less/_repository.less
index 3bbfe33ef..cb6245908 100644
--- a/public/less/_repository.less
+++ b/public/less/_repository.less
@@ -237,7 +237,8 @@
                     &.octicon-mail-reply {
                         margin-right: 10px;
                     }
-                    &.octicon-file-directory, &.octicon-file-submodule {
+                    &.octicon-file-directory, &.octicon-file-submodule,
+                    &.octicon-file-symlink-directory {
                         color: #1e70bf;
                     }
                 }

After applying, run npm i && make generate-stylesheets to generate the CSS.

@bkcsoft bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 29, 2018
@tf198
Copy link
Contributor Author

tf198 commented Apr 30, 2018

Have applied the css patch from @silverwind - looks pretty smart now!
Regarding the second suggestion, any implementation would have to track the paths seen to avoid infinite loops [ link_a -> link_b -> link_c -> link_a ]. Not sure the benefit justifies the complexity...

@thehowl - I am corrected, github currently allows editing of symlinks but gitea doesn't (existing behaviour). Suggest a new issue is opened for modifying this behaviour.

@thehowl
Copy link
Contributor

thehowl commented Apr 30, 2018

We could just avoid nested symlinks (if the target is a dir then it's a dir, otherwise if it's a file, symlink, or anything else, it's a file). Hacky, sure, but should suffice most use cases.

@bkcsoft bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 30, 2018
@lafriks lafriks merged commit 85d14cc into go-gitea:master May 1, 2018
@tf198 tf198 deleted the symlink_icons branch May 1, 2018 23:15
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label May 10, 2018
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants