Skip to content

Show matching icons based on the filename in the repo tree view #24147

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

Closed
wants to merge 24 commits into from

Conversation

yardenshoham
Copy link
Member

@yardenshoham yardenshoham commented Apr 15, 2023

Before

image
image

After (if you set ui.FILE_ICONS = file-specific in app.ini)

image
image

@yardenshoham yardenshoham added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/ui Change the appearance of the Gitea UI labels Apr 15, 2023
@yardenshoham yardenshoham added this to the 1.20.0 milestone Apr 15, 2023
@yardenshoham
Copy link
Member Author

The diff is actually pretty small if you don't count the generated files public/img/*.svg and asset/material-icons.json

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 15, 2023
@yardenshoham yardenshoham marked this pull request as draft April 15, 2023 20:12
@yardenshoham yardenshoham marked this pull request as ready for review April 15, 2023 20:16
@yardenshoham yardenshoham force-pushed the issues/11149 branch 4 times, most recently from fdb7ba5 to 6a286be Compare April 15, 2023 20:38
Signed-off-by: Yarden Shoham <git@yardenshoham.com>
@lafriks
Copy link
Member

lafriks commented Apr 16, 2023

Test failure is related

@yardenshoham yardenshoham marked this pull request as draft April 16, 2023 09:38
@delvh
Copy link
Member

delvh commented Apr 16, 2023

Getting this PR merged will be fun as the two opposing views clash with each other in this PR:
The ones who want this change, and the ones who want to keep Gitea minimal.
I'm somewhere in between.
For me, the usefulness probably outweighs the extra resource usage.

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Before you do anything else, @lunny and I meant this a bit different.
Here's my temporary review.

yardenshoham and others added 3 commits April 16, 2023 16:50
Remove unused struct
Fix test
Co-authored-by: delvh <dev.lh@web.de>
@yardenshoham yardenshoham marked this pull request as ready for review April 16, 2023 16:51
@codecov-commenter

This comment was marked as off-topic.

@delvh delvh self-requested a review April 18, 2023 13:14
@yardenshoham
Copy link
Member Author

By that logic, we can drop all SVGs...

@delvh
Copy link
Member

delvh commented Apr 18, 2023

Well… Yes. But that is basically a PR of its own.


As I've already written on Discord, I'm also fine with marking the functionality as deprecated/experimental and subject to change, and then refactoring the mechanism after this PR has been merged.
Given those circumstances, I can re-approve.


(The funny thing is: With that, the default value of basic suddenly makes more sense as users then have to opt-in into a potentially breaking mechanism)

@yardenshoham
Copy link
Member Author

Experimental is fine I guess

@wxiaoguang
Copy link
Contributor

If we agree to use "icon pack" solution eventually, does it need to merge these 600+ SVG files now? We would see: this PR: +660 SVG files, next PR: -660 SVG files. 😄

@lunny
Copy link
Member

lunny commented Apr 18, 2023

We'd have to build a whole system to map filenames to icons. This is better, simpler, and we don't force it on anyone (it's a setting)

But how to handle these cases:

* Some files are recognized wrong and use non-ideal icons.

* Some users want to use other style icons (eg: do not use icons for some directories).

* Some private instances may have their own private file types and need private icons.

My proposal could include the "rules" json file, keep the icon-detection algorithm here

To be "simple", Gitea could provide an icon package officially, users just need to download it.

Good idea. Could that become a part of theme?

@lunny lunny dismissed their stale review April 18, 2023 13:39

Sorry, I think @wxiaoguang 's idea is better and plugable.

@yardenshoham
Copy link
Member Author

I understand this approach will not be accepted

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 18, 2023
@lunny lunny removed the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Apr 18, 2023
@wxiaoguang
Copy link
Contributor

@yardenshoham if you don't mind, I would try to implement "icon pack" approach, and re-use your code (co-author), does it look good to you?

@yardenshoham
Copy link
Member Author

Yes, go ahead

@lunny lunny removed this from the 1.20.0 milestone Apr 18, 2023
@silverwind
Copy link
Member

silverwind commented Apr 18, 2023

I wouldn't mind if we ship a default, unopinionated pack of icons that can be turned off via an option. Requiring extra configuration and especially defining a plugin interface for icons seems too much work and it would probably not see much adoption by the community I guess.

@yardenshoham
Copy link
Member Author

That is to say, the approach in this PR was good enough?

@lunny
Copy link
Member

lunny commented Apr 18, 2023

I wouldn't mind if we ship a default, unopinionated pack of icons that can be turned off via an option. Requiring extra configuration and especially defining a plugin interface for icons seems too much work and it would probably not see much adoption by the community I guess.

But if users could put their icons on custom/, it could be part of the theme which is attractive.

@yardenshoham
Copy link
Member Author

Still, having the material icons by default would look good to most people that need no customization

@silverwind
Copy link
Member

I can accept this PR. I do not see icons as part of the theme, but if we can make them overridable via custom folder, it's a nice bonus.

I wouldn't expect any community themer to keep up with this task. Assuming we are going to frequently update the icons, it will be a herculean task to keep a custom icon set in sync with our base, which if it is the material one, seems to add new icons every week.

@yardenshoham
Copy link
Member Author

I'm not gonna reopen because I think the majority will block

@silverwind
Copy link
Member

I like the "icon pack" idea. Add files with the name cpp.svg or something in a directory and afterwards .cpp files get that icon. By default that folder is empty which results in the default icon. Then everyone can add the desired icons or keep it empty. This wouldn't bloat compiled Gitea too.

It won't be so simple in many other cases where regex match is needed. That is one of the reasons why I don't see this as suitable for themeing.

I don't see it as bloating, I see it as a nice optional enhancement to the UI. What is the total file size of the icons?

@wxiaoguang
Copy link
Contributor

I am trying to implement a "custom" approach based on yardenshoham's solution.

Let's see if my proposal could work. If I my proposal has problems, let's continue here.

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 18, 2023

The icon pack could contain similar logic as this PR and adding the icons is the "enable" flag.

@yardenshoham
Copy link
Member Author

What is the total file size of the icons?

2.8M    node_modules/material-icon-theme/icons
160K    assets/material-icons.json

@delvh
Copy link
Member

delvh commented Apr 18, 2023

It won't be so simple in many other cases where regex match is needed

That hasn't even been implemented with the approach proposed in this PR, so I don't see a reason to support it at all.
How often does it happen that you actually need a regex to describe which files it applies to?
The only one that comes to mind is ya?ml.
And that simply needs the same icon for the two different values.

@yardenshoham
Copy link
Member Author

I'm sure that the material theme is good enough for 80%+ of users and if not, it can be disabled...

@silverwind
Copy link
Member

silverwind commented Apr 18, 2023

It won't be so simple in many other cases where regex match is needed

That hasn't even been implemented with the approach proposed in this PR, so I don't see a reason to support it at all. How often does it happen that you actually need a regex to describe which files it applies to? The only one that comes to mind is ya?ml. And that simply needs the same icon for the two different values.

I hadn't checked in detail how the matching worked (because the diff view is so slow), but it didn't seem like a simple "endsWith" check at first glance, but maybe it actually is.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/ui Change the appearance of the Gitea UI type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Show matching icons based on the file type
9 participants