Support a little more data in .info files

jenlampton commented 5 years ago

This is a spin-off from https://github.com/backdrop-ops/backdropcms.org/issues/100

I have added several fields to the project content types on backdropcms.org:

1) unlimited value term reference field "Project Tags" -- the list of tags has been seeded with the tags listed in the documentation 2) unlimited value image field "Screenshots" -- this will replace our "Image" field since it is multiple value. The image we pass to project browser will be the first (delta = 0) of this list. 3) unlimited value short text field "GitHub Maintainers" -- since right now all we have are maintainers on GitHub, this was the safest way to handle these values. If in the future we support other kinds of maintainers, an update hook will be easy if we treat all values the same way right now. 4) unlimited value List (text) field "Theme Colors" -- this currently supports any of the following colors:

Aqua
Beige
Black
Blue
Brown
Gold
Gray
Green
Ivory
Lavender
Lime
Maroon
Navy
Olive
Orange
Pink
Purple
Red
Salmon
Silver
Tan
Teal
Violet
White
Yellow

I tried to pull all the un-ambiguous color names from the list of CSS colors, but let me know if we need to pair this down further. (do we really need either/both lavender and violet? what about Tan and Beige? Acqua and Teal?)

~~PR: https://github.com/backdrop-ops/backdropcms.org/pull/488~~ https://github.com/backdrop-ops/backdropcms.org/pull/750

docwilmot commented 5 years ago

@jenlampton @quicksketch PR up, but caveat: I cant really test it. Like I mentioned, I could create fake GitHub API event to test locally but haven't actually been able to figure out how to do a real webhook to WAMP, and had to comment out all mention of backdrop_http_request() during testing locally. So would be grateful if you could handle the finishing touches.

The actual updating of fields works though.

Also note the https://github.com/backdrop-ops/backdropcms.org wasnt updated after you added the new fields to the content types. So I dont know how that will affect merging, but look out for that.

quicksketch commented 5 years ago

I left some feedback at https://github.com/backdrop-ops/backdropcms.org/pull/488#pullrequestreview-187168653.

One thing that's not clear to me: what happens if screenshots[] is specified in a theme's .info file AND there is a screenshot.png file. Are both used? Or does screenshots[] take precedence? I would think that if specified, screenshots[] would be used on backdropcms.org. The screenshot.png in the theme directory would then only be used on the Appearance page.

docwilmot commented 5 years ago

Feedback is all very reasonable, will do shortly.

Are both used?

I left in the code for screenshot.png since it may take a while for themes to be updated (not that we have that many) but I imagine the new screenshots[] deprecates that. I suspect @jenlampton will need to do a .tpl.php file for the screenshots field with a if (!screenshots): screenshots = screenshot.png

We'll need to update Backdrop core to do the same.

jenlampton commented 5 years ago

I suspect @jenlampton will need to...

👍 can do!

jenlampton commented 5 years ago

A thought: we'll need the packager to remove the screenshots directory after sending the files over to backdropcms.org.

docwilmot commented 5 years ago

I didnt remember about this detail.

That would require a change in the thinking behind the code. Currently screenshots[] = CAN_BE_ANY_DIRECTORY/image.png_or_jpeg. We'll need to only delete the images.

But we'd need to think through how the admin/appearance page uses this. Do we allow BackdropCMS to pick a random one of the images and package that with the upload? Or recommend at least one image have a special name that would tell the packager to ship it with the upload ( screenshot.png for example)? Or do we forego an upload completely and just embed an image directly from BackdropCMS (like Installer does, but then if youre offline there'd be no preview)?

For now, since we're keeping screenshot.png most themes would have no problem, but we'd need to decide before we deprecate that.

docwilmot commented 5 years ago

We also cant do this with any existing hooks. We could add a backdrop_alter() to https://github.com/backdrop-ops/backdropcms.org/blob/7e85f08ec395caa6098d2f6149f52d447bdc620d/www/modules/contrib/project/project_github/project_github.pages.inc#L55 to modify the $files before theyre uploaded. This would require a new release of Project module first I imagine. @jenlampton or @quicksketch will need to decide on this.

klonos commented 5 years ago

I would prefer if there was a single screenshot file left as fallback when offline.

docwilmot commented 5 years ago

Can we get feedback on this? @quicksketch can you comment on any way to make Project clean up the screenshots apart from adding new hook(s) to Project?

I would prefer if there was a single screenshot file left as fallback when offline.

My idea was:

allow screenshots in any directory with the screenshots[] = where to locate them
for now for the appearance page, use screenshot.png from the root of the theme package as usual
eventually GitHub would choose an image for the appearance page
- use an image called screenshot.png if it exists in the theme root
- use an image called screenshot.png if it exists anywhere else
- if not use the first image in the screenshot[] = list
GitHub packager could then move that image to the root as usual so all themes downloaded from GitHub would have that as usual. There would be a discrepancy though in that modules on Github would have all images in the theme, but downloaded images would only have one screenshots.png.
the appearance page would always then continue to use screenshots.png
Project Installer would be able to display all the screenshots still by pulling them from BackdopCMS.org directly

jenlampton commented 5 years ago

Do we allow BackdropCMS to pick a random one of the images and package that with the upload?

I think it would be smarter to always pick delta = 0. People will learn that the first image they provide is the one that will be used.

allow screenshots in any directory

Since we are encouraging everyone to use the same directory names in all projects for css js config and libraries I don't see why we shouldn't just assume (and enforce) use of a screenshots directory. It will make for better DX if it's consistent across all projects.

The list of screenshots[] would then contain the file names (within the screenshots directory), and the order would determine the delta. The first image in this list would always be the one used as the screenshot on the project page, and would be the one image that's not removed by the packager. Or it could even be renamed screenshot.png and placed in the project root if that file did not already exist.

docwilmot commented 5 years ago

this is reasonable. now just to figure out https://github.com/backdrop-ops/backdropcms.org/issues/487#issuecomment-457367120, how to get Project to delete images. As mentioned I cannot see how it could be done neatly without changes to Project module.

docwilmot commented 5 years ago

@jenlampton I finished this, then had a thought: if we're going to mandate a screenshots directory, whats the point of the screenshots[] = xxx array?

The way theme info files work now is that if you dont want your screenshot to be called screenshot.png or if you dont want it in your theme root, you can put screenshot = path/to/it in your info file. So people are going to be used to be able to have their screenshots where they wish, and declare this in their info files. Like is you want to keep your screenshot in your images folder, you can do that now in D7 and Backdrop.

So this is why I had though that screenshots[] = path/to/them would work the same.

In either case if we want to mandate a screenshots directory, this becomes superfluous.

jenlampton commented 5 years ago

Interesting. I was thinking that the first one listed would be the primary one (the one that shows up in the project installer, etc) but maybe we could allow a single line that defines only the featured one?

docwilmot commented 5 years ago

So enforce the screenshots folder, AND require a line in the info file to say which one it's the main screenshot?

That would be changing expected functionality a lot though.

jenlampton commented 5 years ago

That would be changing expected functionality a lot though.

Would it? We already have a line in the info file to say which one is the main screenshot.

All we'd be doing is adding/enforcing a screenshots folder for all the other images.

docwilmot commented 5 years ago

by this I mean:

we will lose the ability to specify a preferred location for screenshots and force the screenshots folder as the only option
we will now require an entry in the info file whereas it was previously optional, since there is no way to "order" images in a folder, except alphabetical

personally I dont think this is a major issue since I doubt 80% of theme developers are actually utilizing the first point above, and the second point is going to be necessary regardless, if we're allowing multiple images. but thought it needed saying that we'd be losing existing (if not expected) functionality.

docwilmot commented 5 years ago

I just thought as well though. Its probably likely that the image a theme uses for the screenshot may not be the one we want to show on b...org, since its going to be low resolution? so the screenshots array in the info file is still necessary, so theme devs can decide what order their images on b..org show up.

and do we mandate a maximum number of images? we dont want theme listings on b..org having 16 images.

more questions exist, i'm sure.

@klonos?

jenlampton commented 5 years ago

we will now require an entry in the info file whereas it was previously optional, since there is no way to "order" images in a folder, except alphabetical

We can still leave it as optional, alpha is a suitable fallback. If someone notices the wrong default image is showing they will likely look into how to change it.

Another thought: what if they want their screenshots to all be in a specific order? Do we want to make them rename their files so that they alphabetically match their preferred order? Or would it be better to have them define all of them as part of a screenshots[] array?

docwilmot commented 5 years ago

Another thought: what if they want their screenshots to all be in a specific order? Do we want to make them rename their files so that they alphabetically match their preferred order? Or would it be better to have them define all of them as part of a screenshots[] array?

you might have missed my last comment:

so the screenshots array in the info file is still necessary, so theme devs can decide what order their images on b..org show up.

jenlampton commented 5 years ago

You might have missed my reply to your last comment:

We can still leave it as optional, alpha is a suitable fallback. If someone notices the wrong default image is showing they will likely look into how to change it.

My recent question was about all the other screenshots. :)

docwilmot commented 5 years ago

Summary for anyone reading this (and to make it clearer in my mind):

We are planning to have more info in .info files, and thats come along nicely, we're almost there. One new sugegstion was to have a screenshots[] = xxx array to allow multiple screenshots for display on backdropCMS.org.

But the plan will be that once a release of a theme is made, we will delete the screenshots folder from GitHub so that users downloading the theme from GH wont waste bandwidth on images their site will likely never use as there will still only be one screenshot on the site themes page. Questions arise though:

currently you can add a line in your info file to specify what the name and location of you screenshot is. Do we want to do the same for the screenshots folder? Or for individual screenshots?
or do we mandate a screenshots folder in your theme root, with the screenshot list in the info file and deprecate the ability to have your screenshot stored wherever you want?
how do we tell sites downloading this theme, which image is the standard site admin/appearance screenshot in our array and which ones we want to display on BackdropCMS.org? We were thinking the first one in the array would be the screenshot, but the screenshot would need to be small resolution (its usually only 294 x 219px but doesnt have to be) so would we skip it on BackdropCMS.org?
(just thought of this) If I create a theme foo, one master branch only, and create a release, how do we delete the screenshots folder from GH wihtout a commit to note that we did that? And if the folder is gone, what happens next release? how would BackdropCMS.org handle the (now missing) screenshots folder. I suspect I dont really know how this could all work.
would we want a maximum number of images?

Anything else

ghost commented 5 years ago

Thanks for the summary @docwilmot!

I see two main issues here:

Where to store screenshots
How to determine which is used where

For the first issue, I really prefer consistency over (most) other things. The Creating modules API page says:

JavaScript files are always kept in a /js folder, CSS in a /css folder, templates in a /templates folder, and tests in a /tests folder, as demonstrated in My Module above.

Granted, this is referring to modules, but it should (if not does) apply to themes too. Therefore, if we mandate a specific directory to use for CSS, JS, etc. files, we should do the same for screenshots (consistency FTW).

As for the second issue, screenshots that appear on BackdropCMS.org are just that - screenshots showing the theme in action; how it looks at different screen sizes, etc. But often what will be shown for a theme on the admin/appearance page in Backdrop itself will be a logo, or part of the design; generally not a full-page screenshot.

I therefore propose having two settings to account for this difference:

screenshots[] = xxx: an array of large images in the screenshots folder that are used on BackdropCMS.org for advertising the theme, and which are removed/not added to the zip packages on GitHub.
image = yyy: a single, small logo/cropped screenshot/image in the theme's root folder which is displayed on the admin/appearance page in Backdrop when enabling the theme.

docwilmot commented 5 years ago

Thank you @BWPanda thats been quite helpful. I agree with your evaluation re the screenshots folder (which is what I think we were leaning towards).

Re the image for the appearance page, I think a simple solution could be adding array keys, like screenshots[default] = xxx or screenshots[logo] = xxx. I'll code that and we'll work on the key name later.

Reminder we'll have to remove the code that allows moving the screnshot in a core PR, once this is done.

And never mind re my fourth question about, that isnt a problem, its the release zip, not the repo that we're fiddling with, so all is well there.

docwilmot commented 5 years ago

@jenlampton please see latest PR on this issue. I think I've gone as far as I can without being able to test this. And reminder I cant test this because I dont have the GitHub webhooks pointing at my local.

jenlampton commented 5 years ago

@quicksketch can you let us know how we can test this? When you tested this last time, what process did you use (and should we do the same?)

klonos commented 5 years ago

Sorry for coming late to the party...

and do we mandate a maximum number of images? we dont want theme listings on b..org having 16 images.

Why not?

Another thought: what if they want their screenshots to all be in a specific order? Do we want to make them rename their files so that they alphabetically match their preferred order? Or would it be better to have them define all of them as part of a screenshots[] array?

you might have missed my last comment:

so the screenshots array in the info file is still necessary, so theme devs can decide what order their images on b..org show up.

So I think that:

If people simply add images to a screenshots directory, then we pull these and render them in alpha (as the parse order). This provides an easy, kinda automated way, for those not fussed about the order of the screenshots.
If the maintainers care about the order of their screenshots, then they can manually set it by the optional screenshots[] = lines in their .info files.

...the plan will be that once a release of a theme is made, we will delete the screenshots folder from GitHub so that users downloading the theme from GH wont waste bandwidth on images their site will likely never use as there will still only be one screenshot on the site themes page. Questions arise though

Do you mean that the plan is to have the packaging script remove those from the .zip? If so, then yes; if you literally mean delete the files from GitLab, then please no.

...if we mandate a specific directory to use for CSS, JS, etc. files, we should do the same for screenshots (consistency FTW).

Yes please 👍

klonos commented 5 years ago

...never mind re my fourth question about, that isnt a problem, its the release zip, not the repo that we're fiddling with, so all is well there.

😌 ...yes. This is the way to go: leave the actual files on the repo intact; clean things up in the package.

Re which image should be the default/logo, I think that:

Any image with a specific filename (say thumbnail.whateverextension ??) should serve as what is shown in admin/appearance, and in the theme listing in the Project Browser, and on b.org.
If no thumbnail.xyz file is found, then whatever is delta = 0 from those other images in /screenshots

stpaultim commented 3 years ago

I really like the intent of this PR. I am particularly interested in the idea of including maintainers (#3 in original proposal), if that is still included.

It's not clear how I can help with this issue. Maybe we need another infrastructure sprint week sometime soon when we can focus on issues like this.

It sounds like the hold up might be finding a way to test this PR. Was there some talk about using Tugboat to test PR's for backdropCMS.org?

ghost commented 3 years ago

Was there some talk about using Tugboat to test PR's for backdropCMS.org?

Yes. The Forum repo now has Tugboat PR sandboxes. The API and B.org repos are yet to come.

docwilmot commented 3 years ago

So I got ngrok working locally after some coaxing and tested the PR again. Everything until the screenshots works fine (so I assume this will work fine too). I should be able to push a well reliable PR shortly. But then I am confused again as to how to handle screenshots. The PR expects both a screenshots folder and entries in the .info file listing the filenames. But if we mandate a specific location for screenshots, then the info file entries (screenshots[]='' etc) are redundant I'm thinking. Those info file entries were only considered necessary because I initially considered allowing users to put their screenshots anywhere and then tell where they are through screenshots[]=path.

So how about the following options:

project has no screenshots folder
- the screenshot.png file will be in the root, and if not available whatever fallback exists now
project has a screenshots folder
- screenshot.png is optional. user can specify an entry in the info file saying screenshot = file.png to use one from the folder as default.
- if no default specified, use the "first" one in the screenshots folder.
- if a screenshot.png is also present in the folder or in the root, use that as default.

TO explain the concept of a default in case its not clear, there needs to be a file that's used on the /appearances page; it needs to be a specified size; currently Backdrop will use the file named screenshot.png. But if we're now going to allow multiple screenshots, and most will be large images to display on the BackdropCMS.org theme listing, we need a way to tell the packager which one is the little one to use from the screenshots folder. Thats what I'm calling "default". During release, the packager will create this file from one in the folder either if specified in the info file or the "first" image if not specified. All other images, including the screenshots folder in entirety will be deleted from releases though present in the GH code.

ANy objections?

klonos commented 3 years ago

I think I like the 2nd option, but let me see if I get things right. This is how I'd expect things to work re screenshots:

if there is a screenshot.png file present in the root folder, it is used as default (same as favicon.ico) ...question though: does it have to be a .png, or screenshot. + any acceptable image format/extension would work?
if no screenshot.png file present in the root folder, then look for a screenshots folder
if screenshots folder exists, look for images in it
if there is a screenshot.png file present in the screenshots folder, use that as default
if there is no screenshot.png file present in the screenshots folder, use the "first" as default
the rest of the image files in the folder may be used as a slideshow (in the future - separate issue)
screenshots[]=path specified in the .info file allows specifying/overriding the order of screenshots
the first image in the screenshots[] array is used as default - the rest as slideshow

In other words, screenshots[] in the .info file becomes optional (but still supported) ...the default image is found based on the above logic, plus then we also give people the means to customize/override certain things by still supporting screenshots[] overrides in the .info file.

it needs to be a specified size;

Could we convert this for people, or use image styles or something? (I understand that in order to do that, the file would most likely need to be converted to a managed file in the CMS)

During release, the packager will create this file...

Keep in mind that this will need to work with custom themes (in in the contrib GitHub repo - no releases). Right?

ghost commented 3 years ago

@docwilmot Can we keep it really simple and:

Use images in the /screenshots folder for B.org (this folder and contents will be removed for packaged downloads)
Use /screenshot.png for Appearance page (same as currently - no screenshot file, no thumbnail)

In other words, leave the current functionality in place regarding the screenshot thumbnail (don't change anything about it), and just add the ability to add a folder of images specifically for use on B.org?

klonos commented 3 years ago

just add the ability to add a folder of images specifically for use on B.org?

The project installer within the CMS should also have the capacity to show theme/module screenshots. Will it be pulling it from b.org?

ghost commented 3 years ago

The project installer within the CMS should also have the capacity to show theme/module screenshots. Will it be pulling it from b.org?

Where does it pull information from currently?

klonos commented 3 years ago

Where does it pull from currently?

b.org I believe; as it should 👍

ghost commented 3 years ago

Can we keep it really simple...

Themes already have the ability to specify a screenshot file for use in Backdrop itself (on the Appearance page). This can be screenshot.png in the theme's root directory, or specified elsewhere using the info file setting. Leave all that as-is, it works well.

For this PR, just add support for a screenshots folder which has all the extra images for B.org (etc.?). No weird logic for checking the default image and all that.

That seems the simplest solution to me.

stpaultim commented 3 years ago

I would vote for the #2 option from @docwilmot. I don't feel strongly about the minor issues that are being discussed within the context of that option. I will say.

1) I like the idea of a developer having the option to choose a default image in the .info file (but not requiring it). 2) I like the idea of keeping screenshots in a single directly, but understand that we might not be able to require this - given existing themes that are not structured this way. I don't think it's a problem if a single screenshot is allowed outside of this directory.

Thanks for the work on this issue.

ghost commented 3 years ago

I think I like the 2nd option

I would vote for the #2 option

My interpretation of @docwilmot's comments wasn't that there were two options to choose from, but rather "here's what would happen in these two scenarios"... Am I missing something?

docwilmot commented 3 years ago

My interpretation of @docwilmot's comments wasn't that there were two options to choose from, but rather "here's what would happen in these two scenarios"... Am I missing something?

And you were right. Sorry my "options" were ambiguous.

For this PR, just add support for a screenshots folder which has all the extra images for B.org (etc.?). No weird logic for checking the default image and all that.

Simplicity certainly. I think though future theme developers may be confused if we require a root themeshot.png when there is a directory called screenshots sitting right there next to it. I imagine someone might wonder why the heck you cant put the screenshot in the screenshots directory. So I'm recommending allowing this. Furthermore, a newcomer may also baulk at the suggestion that we require a file named screenshot in a directory full of screenshots! Hence recommending the ability to say in the info file which one has a particular purpose, seeing as they are all screenshots.

screenshots[]=path specified in the .info file allows specifying/overriding the order of screenshots

@klonos I should have re-read the issue, thanks. Forgot we'd had discussions. Yes, the info file screenshots array will be optional, for cases who dont care about image order. Then a theme where dev didnt care about image order would only need one screenshots[default]=path; I think that would also be less confusing than just a single screenshots[]=path when there is a directory full of images.

Remember finally that regardless of what this PR does, the packages that download from GH would still look the same in the end as it does now: a screenshot.png in the root and no screenshots folder. THis PR just describes the magic that happens on first publishing the release.

ghost commented 3 years ago

I think though future theme developers may be confused if...

There's already the ability to put screenshot.png in a different folder e.g. 'screenshots', or to rename it. From the API docs:

The optional screenshot key tells Backdrop where to find the theme's thumbnail image, used on the theme selection page (admin/build/themes). If this key is omitted from the .info file, Backdrop uses the "screenshot.png" file in the theme's directory.

Use this key only if your thumbnail file is not called "screenshot.png" or if you want to place it in a directory outside of your theme's base directory (e.g. screenshot = images/screenshot.png).

So I don't see why your PR needs to be complicated by duplicating this 'magic' logic...

docwilmot commented 3 years ago

Not duplicating, simplifying. So lets suppose you had a screenshot called panda.png. If we keep the current magic unchanged, you'd need in your info file screenshot = screenshots/panda.png (singular) and you would also need an array of screenshots[] = bigpanda.png (plural), screenshots[] = littlepanda.png, etc. We need to keep the option to allow the screenshot in a different folder for legacy reasons though, so this PR doesnt change that, but it allows a simpler option.

docwilmot commented 3 years ago

New PR, simplified. Works using NGrok locally, but we should test on a real dev site, not my old laptop. All updating of the project node with info file data works, deleting the screenshot folder works. I simplified the screenshot handling to do the following:

if there is a screenshots[default] = filename.ext in the info file, copy filename to the root, rename it to screenshot.ext
if there is an actual screenshot.png in the screenshots folder, copy to the root.

The above is necessary because the screenshots folder will be deleted. However, the actual renaming/copying doesn't work locally because ngrok somehow results in multiple webhook triggers, and causes a conflict to saving the project node on creating a release, with a MySQL error. If I trigger ngrok to resend the same webhook (just one) without actually releasing, there is no error, but then the packager wont work because its not an actual release. a catch 22 thing, which is ngroks fault and I suspect should not happen (fingers and toes crossed) without ngrok in between the webhook triggering. But realistically, if it doesn't work, we can forget all about the image swapping for the initial commit and just require devs to ensure they have a screenshot.png in the root, as now. So since everything else works, including deleting the screenshots folder, I believe this is good to go!

docwilmot commented 3 years ago

@jenlampton just realised we might not need a fresh demo site, if you create a webhook pointing at the Tugboat site https://pr750-qenkymweoirm66ephnicnjztpozqu0rh.tugboat.qa/. Not sure if that'd work, but worth a try. Could finally get this tested.

jenlampton commented 3 years ago

The hook request has to come from the site itself. I think from the Project GitHub module UI?. I can approve it once the request appears in GitHub. (I think the instructions are in the readme for project_github module)

docwilmot commented 3 years ago

Ahh, I see. For my testing I just created a webhook manually on GH. Will try this.

docwilmot commented 3 years ago

Listened to the last dev meeting and sounds like @quicksketch is recommending spoofing releases and webhooks to test, which is what I'd done, using Ngrok. So this could use a code review then should be ready I'd say.

quicksketch commented 3 years ago

We mentioned this last week and again today that we should go ahead and get this deployed onto the live environment for further testing. As this goes directly into the backdropcms.org repository, we should plan on getting this merged and deployed all-together.

jenlampton commented 3 years ago

I would like to merge the current PR, which adds support for:

colors
screenshots
maintainers
tags

But it looks like it needs a rebase? It's got some conflicts. @docwilmot

docwilmot commented 3 years ago

You may have tried to merge the oldest PR, which I forgot to close. The latest is https://github.com/backdrop-ops/backdropcms.org/pull/750

jenlampton commented 3 years ago

Yep, sorry about that. Merged that one! :)

backdrop-ops / backdropcms.org

Support a little more data in .info files #487