Skip to content

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Oct 2, 2025

Summary

One step to get rid of Karma tests (aka "jsunit").

Checklist

@susnux susnux requested a review from a team as a code owner October 2, 2025 21:13
@susnux susnux added the 3. to review Waiting for reviews label Oct 2, 2025
@susnux susnux requested review from nfebe and removed request for a team October 2, 2025 21:13
@susnux susnux added the ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) label Oct 2, 2025
@susnux susnux requested review from sorbaugh and szaimen October 2, 2025 21:13
@susnux susnux added this to the Nextcloud 33 milestone Oct 2, 2025
@susnux susnux requested review from CarlSchwan and skjnldsv October 2, 2025 21:13
@susnux susnux force-pushed the chore/karma-mimetype branch 2 times, most recently from c731157 to b94a1cd Compare October 2, 2025 21:16
susnux added 2 commits October 6, 2025 14:16
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux force-pushed the chore/karma-mimetype branch from b94a1cd to 7bda1cd Compare October 6, 2025 12:18
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

🐘

@susnux susnux merged commit 678a8a7 into master Oct 6, 2025
209 of 213 checks passed
@susnux susnux deleted the chore/karma-mimetype branch October 6, 2025 13:05
Comment on lines +38 to +48
if (window.OCA.Theming && gotIcon === null) {
path = generateUrl('/apps/theming/img/core/filetypes/')
path += getMimeTypeIcon(mimeType, window.OC.MimeTypeList.files)
gotIcon = true
}

// If we do not yet have an icon fall back to the default
if (gotIcon === null) {
path = generateUrl('/core/img/filetypes/')
path += getMimeTypeIcon(mimeType, window.OC.MimeTypeList.files)
}
Copy link
Contributor

@ShGKme ShGKme Oct 22, 2025

Choose a reason for hiding this comment

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

It is not only moving the function but also changing it
(using suggestion just to show a diff)

Suggested change
if (window.OCA.Theming && gotIcon === null) {
path = generateUrl('/apps/theming/img/core/filetypes/')
path += getMimeTypeIcon(mimeType, window.OC.MimeTypeList.files)
gotIcon = true
}
// If we do not yet have an icon fall back to the default
if (gotIcon === null) {
path = generateUrl('/core/img/filetypes/')
path += getMimeTypeIcon(mimeType, window.OC.MimeTypeList.files)
}
if (OCA.Theming && gotIcon === null) {
path = OC.generateUrl('/apps/theming/img/core/filetypes/')
path += OC.MimeType._getFile(mimeType, OC.MimeTypeList.files)
gotIcon = true
}
// If we do not yet have an icon fall back to the default
if (gotIcon === null) {
path = OC.getRootPath() + '/core/img/filetypes/'
path += OC.MimeType._getFile(mimeType, OC.MimeTypeList.files)
}

But generateUrl cannot be used here.

  1. /apps/theming/img/ - is a PHP route managed by a controller
  2. '/core/img/filetypes/' - is a static file path

In the first case we need to include index.php when no rewrite. But in the second adding index.php breaks the path to the icon.

/index.php/core/img/filetypes/file.svg is not correct.

It must be either imagePath or generateFilePath, or getRootPath like in the original version. But it cannot be generateUrl

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @susnux

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in #55916

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants