Skip to content

Comments

Could not resolve icon for text/plain#16058

Closed
kesselb wants to merge 1 commit intomasterfrom
bugfix/fallback-for-text-plain-does-not-work
Closed

Could not resolve icon for text/plain#16058
kesselb wants to merge 1 commit intomasterfrom
bugfix/fallback-for-text-plain-does-not-work

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 24, 2019

Screenshot from 2019-06-24 14-28-58

<div class="activity-previews">
    <a href="https://nextcloud.test/apps/files/?dir=/&amp;scrollto=nextcloud-16.0.1.zip.d1561324636&amp;view=trashbin">
        <img class="preview preview-mimetype-icon" src="https://nextcloud.test/apps/theming/img/core/filetypes/text-plain.svg" alt="Open file">
    </a>
</div>

Icon apps/theming/img/core/filetypes/text-plain.svg does not exist. There should be a fallback to plain.svg but imagePath does not check if the icon exists. I'm not sure about this change because this icon resolve logic is rather complex and there might be something broken on my setup ...

@kesselb kesselb force-pushed the bugfix/fallback-for-text-plain-does-not-work branch from dfa5899 to f0e5b66 Compare June 24, 2019 12:41
@kesselb kesselb requested a review from juliusknorr June 24, 2019 12:41
@kesselb kesselb added this to the Nextcloud 17 milestone Jun 24, 2019
@kesselb kesselb added 3. to review Waiting for reviews bug labels Jun 24, 2019
&& file_exists(\OC::$SERVERROOT . "/themes/$theme/core/img/$basename.png")) {
$path = \OC::$WEBROOT . "/themes/$theme/core/img/$basename.png";
} elseif($themingEnabled && $themingImagePath) {
} elseif($themingEnabled && $themingImagePath && file_exists(\OC::$WEBROOT . $themingImagePath)) {
Copy link
Member

Choose a reason for hiding this comment

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

This will never be true since the $themingImagePath is already an URL to the endpoint of the teming app, so file_exists will always fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I mixed-up \OC::$SERVERROOT and \OC::$WEBROOT.

@juliusknorr
Copy link
Member

@kesselb Is this also an issue in the files app or just with activity?

Signed-off-by: Daniel Kesselberg <[email protected]>
@kesselb kesselb force-pushed the bugfix/fallback-for-text-plain-does-not-work branch from f0e5b66 to f37264f Compare June 25, 2019 12:32
@kesselb
Copy link
Contributor Author

kesselb commented Jun 25, 2019

@kesselb Is this also an issue in the files app or just with activity?

Only activity. We are running into this case if getPreviewPathFromMimeType is called.

  1. Upload a txt file
  2. Do some stuff that creates an activity (e.g. share it)
  3. Move the file to the trash bin
  4. Visit the activity app

@juliusknorr
Copy link
Member

@kesselb Please see #16095 since we should check if the file exists before we actually return the url from the theming app imo.

@juliusknorr juliusknorr deleted the bugfix/fallback-for-text-plain-does-not-work branch July 1, 2019 10:35
@juliusknorr juliusknorr restored the bugfix/fallback-for-text-plain-does-not-work branch July 1, 2019 10:35
@juliusknorr juliusknorr deleted the bugfix/fallback-for-text-plain-does-not-work branch July 1, 2019 10:36
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 bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants