Use new filetype icons #1118
Comments
|
Hey @jancborchardt These new icons looks nice. 👍 I can not wait to change the current ones for these. |
|
cc @tobiasKaminsky @AndyScherzinger @stoyicker @Kernald too :) |
|
Nice! This should simply be replacing the actual png files with these and that's it :) |
|
What about the ldpi version? Currently, the images are 36*36 px - which is not a multiple of 16. Should I upscale them to 48 px? |
|
Hm, damn. It’s pixel-perfect at 16_16. Maybe then it’s better to just use 32_32 instead of 36? |
|
@AndyScherzinger can you add these to your material design work for android? |
|
@jasperweiss I generated all the icons in the right size, I'm just waiting for an answer regarding the ldpi size to open a PR. |
|
@Kernald as I said: Use 32 * 32 instead of 36. :) |
|
Alright, I'm not at home right now, I'll open the PR later today. |
|
Great stuff, thank you! :) |
|
should further icons be added? The screenshot of the webapp shows the gear (for bin) and the "<>" for css. This would also be something that could potentially be added to the android app :) |
|
For reference, if additional icons are planned: the mime-type/icon equivalency table from core is here. |
|
For Android the icon calculation can be found here: https://github.com/owncloud/android/blob/master/src/com/owncloud/android/utils/DisplayUtils.java#L160 |
|
@Kernald could you add the other two? Just the icon work, I can work on the bindings :) - Would this be something you would like to be done @rperezb / @jancborchardt ? |
|
I can generate the icons if needed, yes, that's not a problem. Just ping me if they're needed! |
|
Sounds great, just waiting for the others' reply :) |
|
@jancborchardt @rperezb what do you think? Adding the other icons would make sense imho since they will be introduced by oC 8.2 on the web and the App would display another icon in case of the new icons (gear/code) which would lead to an inconsistency. |
|
Well, the icons should be used for the same filetypes as in the web interface. For example, the icon-code is to be used for all kinds of code filetypes, not only for CSS. See the mimetype list table @Kernald posted: https://github.com/owncloud/core/blob/master/core/js/mimetypelist.js |
|
@jancborchardt I am aware of that 😄 I would change the Android code according to the web code. @Kernald can you add the other icons that would be great! 👍 |
|
Sure, I'll generate them tonight |
|
@Kernald sweet! Thanks a lot! |
|
hey, yes, this is a great idea. |
|
@jancborchardt how smart can my detection algorithm be? I ask this since I would incorporate two things from the web app:
Thus match the type according to the way the web app does it, calculate the mimetype for the file extensions based on the web app's definition :) |
|
@AndyScherzinger this is the most current list: https://github.com/owncloud/core/blob/master/core/js/mimetypelist.js |
|
That's the one I referred to ;) |
|
@AndyScherzinger I know. :) I’m not sure about the other one cc @rullzer |
|
imho, the other is used to determine the mime type, based on the file extension :) |
|
Looking at the android code this change will have an impact since the web app code (as of now) detects certain types the android app would after my update. On the other hand the Android app also detects types, like rtf, die web app doesn't... at least not from the above mentioned files. |
|
yes, the mimetypemapping is uses to map extentions to mimetypes. And then here is the logic to go from a mimetype to the right image: https://github.com/owncloud/core/blob/master/core/js/mimetype.js |
|
https://github.com/owncloud/android/blob/master/src/com/owncloud/android/utils/DisplayUtils.java#L162 does the mapping on android, which shows a hacky one for "application" which might simply be due to android (as in operating system) failing to detect some cases and then defaults to application/octet-stream and then tries to determine the correct icon based on the file extension. I'll post the added values later, so they can be added to the web app to, which is what I would recommend for visual consistency. |
|
@rullzer looking at the js code, how do you detect the pdf? |
|
@rullzer running a matcher test for the code leaves the following pair without a nice icon, which seems odd, since I don't think the web app doesn't match mp3, pdf, and oders like that. The list is "extension" / "mimetype" and no separate icon thus always showing the file. kra / application/x-krita |
|
I read too fast, sorry if I missed something important. The general rule should be ** incremental ** improvement:
|
|
I wasn't going to drop android app functionality, just posting the list for @rullzer to decide on what he wants to do with it. I will on the other hand add all mapping neccessary/missing on the android side :) So it is incremental in a functional way, but the detection is a complete rewrite of today's code. |
|
Nice 👍 |
|
One thing I will add due to the amount of extension/mimetypes not being is a simple mapping for a certain amount of main types:
in contrast the following main types won't me mapped as a fallback and thus be shown as file in case the complete mime type couldn't be matched:
|
|
Mapping suggested I am going to implement for the not yet mapped ones (from the comments above): if not explizitly stated it defaults based on the main type kra / application/x-krita |
|
Here is the test run for file extensions, pretty verbose.... kra / file |
|
@davivel @jancborchardt @Kernald @tobiasKaminsky @masensio - please see #1149 for the PR with the new implementation + icons example screenshot showing some of the additional icons now incorporated: |
|
@rperezb just wanted to hint that #1149 which is to some extend part of #1118 is not planned for 1.8 (which is fine I guess), so the additional 8.2 icons (calendar, contact, code) won't be in this release, just the 8.2 icons for the ones already been present in 1.7.2 (but now with the new design). Like I said, not a problem, but I wanted to mention it just in case, since we split the issue into two PRs being: #1124 and #1149 - the later definitely would need some testing since it also made code changes necessary due to the new/additional icons and a new way to decide which icon to uses, aligned with the web app's implementation. |
|
Similar to iOS, the filetype icons are now stuck too far to the left. Can you move them to the right a bit so they are centered between the left edge and the filename? :) |
|
@AndyScherzinger I'm think you are missing something. Lets walk trough the pdf example. If you look at https://github.com/owncloud/core/blob/master/config/mimetypemapping.dist.json you see that for example
So the Now https://github.com/owncloud/core/blob/master/core/js/mimetypelist.js is a cache we have. But the files part is basically all the icons we have. Now the logic is in https://github.com/owncloud/core/blob/master/core/js/mimetype.js More specifically in https://github.com/owncloud/core/blob/master/core/js/mimetype.js#L41-L60. This function takes the mimetype (application/pdf). It replaces the
If none of those mach we return the default icon. But there is an icon application-pdf. So we use that one. Another short example. .mp3 is mapped to I hope this clears it up a bit. But kick me if you need more info. |
|
@rullzer argh, my bet. I totally didn't see the |
|
@jancborchardt kind if center aligned the folder icon, see screenshot If it fits your needs, feel free to merge :)
|
|
@jancborchardt , after moving the icon, the badges slightly overlap them, as shown in the screenshot. Please, confirm that is OK. We'd like to include this into the release 1.8, but to get it we need to stop discussing tiny graphic details ASAP. |
Yes, that was the intention. They are like badges on the file. |
|
👍 , thanks |
From core master: https://github.com/owncloud/core/tree/master/core/img/filetypes
Preview:
They are 16*16px but of course scale them up. Please only in multiples of 16px, for example 48px.
This should be done in sync with the release of 8.2 server. cc @rperezb @MTRichards @davivel @masensio
The text was updated successfully, but these errors were encountered: