Skip to content

MIR-1449 Erroneous association of some file extensions with icons in user interface #1157

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

levshyn
Copy link
Contributor

@levshyn levshyn commented May 6, 2025

In the user interface, when viewing a document in the Files section of the derivative, some extension files are incorrectly associated with icons. Example: the .m extension is associated with an icon for audio files.

Link to jira.

@levshyn levshyn requested a review from Possommi May 6, 2025 11:37
levshyn pushed a commit that referenced this pull request May 9, 2025
- Added Jersey resource to serve external MIME type mapping JSON
- Improved file type detection logic in derivate-fileList.js
- Updated Handlebars template and mycore.properties accordingly
levshyn pushed a commit that referenced this pull request May 9, 2025
levshyn pushed a commit that referenced this pull request May 9, 2025
@sebhofmann sebhofmann self-requested a review May 9, 2025 11:08
Copy link
Member

@sebhofmann sebhofmann left a comment

Choose a reason for hiding this comment

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

The json file and the resource can be replaced by MCRConfigHelperServlet.

  <servlet>
    <servlet-name>MimeTypeIconMappingConfigHelper</servlet-name>
    <servlet-class>org.mycore.frontend.servlets.MCRConfigHelperServlet</servlet-class>
    <init-param>
      <param-name>Properties</param-name>
      <param-value>MIR.Mimetype.IconMapping.*</param-value>
    </init-param>
  </servlet>
  <servlet-mapping>
    <servlet-name>MimeTypeIconMappingConfigHelper</servlet-name>
    <url-pattern>/mime-type-icons.json</url-pattern>
  </servlet-mapping>

Property example:

MIR.Mimetype.IconMapping.fa-file-code=application/vnd.wolfram.mathematica.package

or

Property example:

MIR.Mimetype.IconMapping.application/vnd.wolfram.mathematica.package=fa-file-code

This does not require a new additional json configuration file and a java resource for fixing a bug.

@levshyn
Copy link
Contributor Author

levshyn commented May 9, 2025

This does not require a new additional json configuration file and a java resource for fixing a bug. -> Yes, that's true. But what if the user needs to change/add a lot of icon mappings and contentType(s)? Will they have to add a lot of MIR.Mimetype.IconMapping.* variables? Wouldn't it be easier to just add/change the data in the JSON configuration file? The idea was to allow the user to simply and easily change the mapping icon -> contentType if needed and not just fix a bug.

@sebhofmann
Copy link
Member

This does not require a new additional json configuration file and a java resource for fixing a bug. -> Yes, that's true. But what if the user needs to change/add a lot of icon mappings and contentType(s)? Will they have to add a lot of MIR.Mimetype.IconMapping.* variables? Wouldn't it be easier to just add/change the data in the JSON configuration file? The idea was to allow the user to simply and easily change the mapping icon -> contentType if needed and not just fix a bug.

You can also inverse your argument. If the User just wants to change a single icon, then he needs to copy the json file, just to change one icon. With my approach, you just need to change one property.

In MyCoRe we already use properties to configure the application behaviour. It is well a well documented feature and every MyCoRe user knows it. We add additional code to avoid secondary configuration files like persistence.xml. Adding extra code to support a new undocumented configuration file, seems like big step backward.

@levshyn
Copy link
Contributor Author

levshyn commented May 12, 2025

I agree with your arguments. I'll rework it to read variables from mycore.properties, as you advised.

levshyn pushed a commit that referenced this pull request May 12, 2025
…g., 'Code') instead of icon class names, simplify config via MCRConfigHelperServlet
@levshyn levshyn requested a review from sebhofmann May 13, 2025 06:04
Copy link
Member

@sebhofmann sebhofmann left a comment

Choose a reason for hiding this comment

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

Sorry, for the late response. I would prefer another change. Currently there is a mapping from the property to a icon definition e.G. from MIR.Mimetype.IconMapping.Code=application/vnd.wolfram.mathematica.package

But Code is not directly the fontawesome icon, its just a entry in the predefined table fileIcons. This allows only to use icons defined in the table and it also increases the code complexity, because its another mapping. I would prefer to not have this mapping and just have a property like:
MIR.Mimetype.IconMapping.fa-file-code=application/vnd.wolfram.mathematica.package

This has the following advantages:

  • easier to understand
  • you can use other icons not present in the js file
  • all icons would be in the mycore.properties file

@levshyn
Copy link
Contributor Author

levshyn commented Jun 11, 2025

ok, and how should we determine which tooltip (label) to use? At the moment we have const fileIcons in derivate-fileList.js with JSON structure, which uses following definitions as keys: "PDF", "Archive", "Image"... These keys are currently used as tooltips (labels).
Update: The question is removed, I will just leave the definition of tooltip (label) using the getHeuristicType() function

Oleksiy 'Alex' Levshyn added 6 commits June 11, 2025 15:39
- Fixed some file type detections using exact extension matching
- Added full JSDoc documentation for all helpers
- Refactored to use object keys as labels
- Improved robustness with trim() and strict checks
- Added Jersey resource to serve external MIME type mapping JSON
- Improved file type detection logic in derivate-fileList.js
- Updated Handlebars template and mycore.properties accordingly
…g., 'Code') instead of icon class names, simplify config via MCRConfigHelperServlet
@levshyn levshyn force-pushed the issues/MIR-1449_Erroneous_association_of_some_file_extensions_with_icons_in_user_interface branch from 3f21db0 to 4298fde Compare June 11, 2025 13:40
@sebhofmann
Copy link
Member

The labels are wrong now anyway. If i open the site as a german user i get the label "Image". The best way would be to load all keys with a prefix e.G mir.file.mimetype.label.* with the translate resource and lookup the right labels. First look for the exact type mir.mime.label.application.vnd.wolfram.mathematica.package (replace special characters with .) and if nothing is found look for generic label mir.mime.label.application.

@toKrause toKrause changed the title MIR-1449: Erroneous association of some file extensions with icons in user interface MIR-1449 Erroneous association of some file extensions with icons in user interface Jun 24, 2025
@Possommi
Copy link
Contributor

New PR #1195 addresses the correct mapping of files to icons. Maybe this PR should go into main?

@yagee-de yagee-de marked this pull request as draft August 5, 2025 12:01
@yagee-de yagee-de changed the base branch from 2023.06.x to main August 5, 2025 12:02
@yagee-de
Copy link
Member

yagee-de commented Aug 5, 2025

please rebase branch to main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants