Skip to content

Conversation

@WebFreak001
Copy link
Member

This is a split part of #265, just only for package logos

This commit goes slightly different on the caching name due to the comments, but still keeps all image files. This time the user specifies the revision of the icon and gets the default package if it is not found. If the user does not specify one they get the current logo. I decided to go for this approach due to archiving or embedding purposes, especially on google etc. where an old image of the repository might be wanted.

https://wfr.moe/fCT3SE.png


https://wfr.moe/fCTqNx.png


https://wfr.moe/fCuZTO.png

@MartinNowak @wilzbach

This is a split part of dlang#265, just only for package logos

This commit goes slightly different on the caching name due to the comments, but still keeps all image files. This time the user specifies the revision of the icon and gets the default package if it is not found. If the user does not specify one they get the current logo. I decided to go for this approach due to archiving or embedding purposes, especially on google etc. where an old image of the repository might be wanted.
@dlang-bot
Copy link
Collaborator

Thanks for your pull request and interest in making D better, @WebFreak001! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Left a few comments. I love the new default logo though I'm not sure it's a good idea to use it as defaut logo (in terms of branding). Maybe a greyish or slightly alternated version of it should be used as default logo, s.t. it's clear that it's not the dub logo but a default logo.

img.packageLogo {
display: block;
float: none;
max-width: 100%;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a bit too large on mobile?

Copy link
Member Author

Choose a reason for hiding this comment

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

made it so it isn't higher than 220px now, it can still be a wide logo but square logo will be smaller now. It does not stretch (at least not in chrome)

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we require logos to be square and convert them otherwise? This usually makes such design things a lot easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm I'm actually a fan of supporting flat logos such as banners, isn't that much more work with html and css

/// file = the file to convert
/// name = the logo name to put in uploads/logos
/// deleteExisting = if false, throw an exception if the files already exist
/// deleteFinish = if true, delete the input file after at least one successful conversion
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW DStyle would be /** ... */ and require a body + Returns:

https://dlang.org/dstyle.html

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, unfortnately it wasn't possible so far to persuade Walter into fixing the semantics for this. What this will do is to create one paragraph per line instead of a single one.

/// name = the logo name to put in uploads/logos
/// deleteExisting = if false, throw an exception if the files already exist
/// deleteFinish = if true, delete the input file after at least one successful conversion
auto generateLogo(NativePath file, string name, bool deleteExisting = false, bool deleteFinish = true) @safe
Copy link
Contributor

Choose a reason for hiding this comment

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

Use std.typecons.Flag or a config struct. Otherwise this gets really hard to read on the calling site.

}
catch (Exception e)
{
(() @trusted => send(owner, e.toString()))();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think it should, but the vibe.d's earlier re-implementation before the compatibility additions, didn't have that feature, so all examples are still explicitly passing the owner (opened vibe-d/vibe.d#2091).


string base = buildPath(logoOutputFolder, name);
string pngOutput = base ~ ".png";
auto png = spawnProcess(["convert", file.toNativeString, "-resize", "512x512>", pngOutput]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the >? `
There are no ouput streams.

Copy link
Member Author

Choose a reason for hiding this comment

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

the > makes sure aspect ratio is kept, it's not a pipe and spawnProcess couldn't pipe anyway

- auto title = "Package " ~ packageName ~ " version " ~ versionInfo["version"].get!string;

// facebook & co.
meta(property="og:url", content="http#{req.tls ? `s` : ``}://#{req.host}/packages/#{packageName.split(':')[0]}")
Copy link
Contributor

Choose a reason for hiding this comment

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

how about wrapping the host things into a functions? #{req.registryURL}

// facebook & co.
meta(property="og:url", content="http#{req.tls ? `s` : ``}://#{req.host}/packages/#{packageName.split(':')[0]}")
meta(property="og:type", content="article")
meta(property="og:title", content="Package #{packageName.split(':')[0]} on DUB")
Copy link
Contributor

Choose a reason for hiding this comment

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

splitter and do we really have to use split here?

//- TODO: register dub twitter account
meta(name="twitter:site", content="@D_Programming")
meta(name="twitter:url", content="http#{req.tls ? `s` : ``}://#{req.host}/packages/#{packageName.split(':')[0]}")
meta(name="twitter:title", content="Package #{packageName.split(':')[0]} on DUB")
Copy link
Contributor

Choose a reason for hiding this comment

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

splitter and do we really have to use split here?

- auto logo = packageInfo["logo"].opt("");

- if (logo.length)
img.packageLogo(src=logo, alt="#{packageName.split(':')[0]} logo")
Copy link
Contributor

Choose a reason for hiding this comment

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

splitter and do we really have to use split here?

x="10.739017"
id="image5884"
xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAg0AAAIJCAIAAACp65nVAAAAA3NCSVQICAjb4U/gAAAgAElEQVR4
nO3dyW8beX7//6rivkmkREryItNLe2u7DTc83ZPpZIJJgEGOQc7Jf5BzECT/RoCcghxyCJIAOc0E
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you use inline png here?

Copy link
Member Author

Choose a reason for hiding this comment

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

uhh, dunno what inkscape did there. I had the D logo in there once but it didn't look so good, maybe it still kept it

@s-ludwig
Copy link
Member

Agree with the default logo. Reducing opacity and/or adding a checker pattern or similar could be used to signal that it's a default logo, too.

@s-ludwig
Copy link
Member

s-ludwig commented Feb 28, 2018

BTW, a relatively cheap way to instantly get some non-default icons would be to use the organization avatar on GitHub (not if the owner is a user obviously, though) and the project avatar for GitLab and Bitbucket. Could be a new field in dubregistry.repositories.repository.RepositoryInfo.

Edit: Or rather the org avatar for Bitbucket, too.

WebFreak001 added 8 commits February 28, 2018 21:20
This way wide logos will still stay wide, rectangle logos won't be larger than 220x220px which is roughly half the display width at the breakpoint and a bit more than half the display width on regular mobile displays
@WebFreak001
Copy link
Member Author

@s-ludwig made the default logo gray

@WebFreak001
Copy link
Member Author

I think getting a default logo on registration time or something like that is scope of another PR.

@WebFreak001
Copy link
Member Author

mergable now?

@s-ludwig
Copy link
Member

I would second the question of whether to store logos as files or in the database. For the mentioned reasons I'd lean towards database, it also won't work with the current setup, because the registry runs as an unprivileged user without write access to its own directory.

BTW, does the convert invocation always scale to 512, or will it only do downscaling?

Apart from that I'd say everything is ready.

@WebFreak001
Copy link
Member Author

yeah ok I will check how well that is doable.

convert with > only downscales

Copy link
Member

@MartinNowak MartinNowak left a comment

Choose a reason for hiding this comment

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

Fairly nice so far.

BTW, a relatively cheap way to instantly get some non-default icons would be to use the organization avatar on GitHub (not if the owner is a user obviously, though) and the project avatar for GitLab and Bitbucket. Could be a new field in dubregistry.repositories.repository.RepositoryInfo.

Just scrape og:image from the repo sites initially to bootstrap the logos, then we don't need a repetitive default logo.

Also I would second to use the db instead of the filesystem to store the logos, simplified highly-available setups, backups, and migration of the service.

meta(property="og:description", content=versionInfo["description"].opt!string)
meta(property="og:site_name", content="DUB Package Registry")
meta(property="og:locale", content="en_US")
meta(property="article:author", content=user.fullName.length ? user.fullName : user.name)
Copy link
Member

Choose a reason for hiding this comment

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

Nice :)

- import userman.controller;
- auto title = "Package " ~ packageName ~ " version " ~ versionInfo["version"].get!string;

- auto normalizedPackageName = packageName.splitter(':').front;
Copy link
Member

Choose a reason for hiding this comment

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

findSplit(':')[0]

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage of findSplit?

Copy link
Member Author

Choose a reason for hiding this comment

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

@wilzbach oh actually I just found out that if a packageName were to be empty, this method would crash because split and splitter return an empty array for an empty string...

100 comments on this pr 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but that's not going to happen, is it?
Anyhow, if you are experiencing this problem somewhere else, there you might like orElse

dlang/phobos#5154

I'm also a big fan or safeDeref, but I'm not sure whether the version on the NG supports ranges out of the box.

@s-ludwig
Copy link
Member

Just scrape og:image from the repo sites initially to bootstrap the logos, then we don't need a repetitive default logo.

Didn't realize that they augmented the repo size with that! Nice.

@wilzbach
Copy link
Contributor

FYI: the default github avatars use a "technology" that's called identicon. You can even skip the query to github and create such icon on-demand, e.g.

https://identicon-api.herokuapp.com/dub-registry/200?format=svg

Or a newer service: https://jdenticon.com/#icon-dub-registry

See also:

@MartinNowak
Copy link
Member

MartinNowak commented Mar 11, 2018

FYI: the default github avatars use a "technology" that's called identicon. You can even skip the query to github and create such icon on-demand, e.g.

We would just fetch the logo once for each package initially and on registration of new packages, really trivial and saves a manual step in case the organization/user logo is already the right choice and continues to work across github/gitlabs/bitbucket's feature set (e.g. per-repo logos https://gitlab.com/gitlab-org/gitter).
Extracting meta og:image should be easy with a Regex instead of requiring a full-blown HTML parser.

@WebFreak001
Copy link
Member Author

WebFreak001 commented Mar 13, 2018

logos are now stored inside mongo as bson binary objects, however caching got removed due to this as sendFile isn't used anymore.

have file revs
only show when changed
Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Looks really good!

m_packages.update(["name": packname], ["$unset": ["logo": 1, "logoHash": 1]]);
}

bdata_t getPackageLogo(string packname, out bdata_t rev)
Copy link
Contributor

Choose a reason for hiding this comment

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

This a question of design/personal preferences, but I typically return tuples in such cases. Hence out of interest, why did you go with out?

Copy link
Member Author

Choose a reason for hiding this comment

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

I go struct or out, but never tuple because it requires me to import something rather large. Selective imports don't help with compilation time in most of phobos.

Copy link
Contributor

Choose a reason for hiding this comment

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

std.typecons is already imported in other modules (and even in vibe.d), so there's no extra import cost.
Also FWIW we reduced the total import time of all Phobos modules to < 0.5s with 2.079. On my machine it's even less than 0.2s now

See: https://dlang.org/changelog/2.079.0.html#std-experimental-all

a.put(chunk);
})();

return cast(bdata_t)a.data.idup;
Copy link
Contributor

Choose a reason for hiding this comment

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

Appender uses the GC to allocate plus it's local, so you can use assumeUnique and avoid the idup copy

Copy link
Member

Choose a reason for hiding this comment

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

Or change to appender!(immutable(ubyte)[])

Copy link
Member Author

Choose a reason for hiding this comment

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

is there some way to read all output from a File stream? I don't know any and I basically just took this code from std.process.execute

Copy link
Contributor

Choose a reason for hiding this comment

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

well, in this case you could simply use execute - this would save a few lines?
(I will look into making modifying execute)

Copy link
Member Author

@WebFreak001 WebFreak001 Mar 14, 2018

Choose a reason for hiding this comment

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

execute converts to a string, but the std output gets a binary png file. I don't want to risk an eventual std.utf.validate getting in there.

Copy link
Contributor

@wilzbach wilzbach Mar 14, 2018

Choose a reason for hiding this comment

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

I will look into making modifying execute

dlang/phobos#6276 (for a start)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to risk an eventual std.utf.validate getting in there.

FTR with cast(immutable(ubyte)[]) execute(["convert", ...]) validate isn't called.
Appender uses ElementEncodingType, not ElementType to avoid auto-decoding.

Copy link
Member

Choose a reason for hiding this comment

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

Just for completeness, there is vibe.stream.operations.readAll to read all of a stream into a buffer.

Copy link
Member Author

Choose a reason for hiding this comment

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

does it work for std.stdio.File like you get when spawning a process?

Copy link
Member

Choose a reason for hiding this comment

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

No, would have to be wrapped inside a StdFileStream for that, but that is rather ineffecient currently.

import dub.semver;
import dub.package_ : packageInfoFilenames;
import std.algorithm : canFind, countUntil, filter, map, sort, swap;
import std.algorithm : any, canFind, countUntil, filter, map, sort, swap;
Copy link
Contributor

Choose a reason for hiding this comment

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

Any isn't used anymore

// make sure requester actually supports svg, if a custom IDE or tool for fetching images is used it should only get png if it doesn't support svg.
// if requester is an IDE sending Accept: */*, but not accepting SVG it's their own fault.
foreach (accept; req.headers.get("Accept", "").splitter(",")) {
if (accept.startsWith("image/*", "image/svg", "*/*", "*/svg")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this among?

Copy link
Member Author

@WebFreak001 WebFreak001 Mar 14, 2018

Choose a reason for hiding this comment

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

image/*; q=0.8, image/svg+xml

Copy link
Member Author

Choose a reason for hiding this comment

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

eventually gonna make a PR to vibe.d for proper Accept parsing

void postSetLogo(scope HTTPServerRequest request, string _packname, User _user)
{
enforceUserPackage(_user, _packname);
const(FilePart) logo = request.files.get("logo");
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the braces aren't needed here. In fact, just const should work too.

enforceUserPackage(_user, _packname);
const(FilePart) logo = request.files.get("logo");
enforceBadRequest(logo != FilePart.init);
auto renamed = NativePath.fromString(logo.tempPath.toString ~ logo.filename.name.extension);
Copy link
Contributor

Choose a reason for hiding this comment

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

NativePath.fromString should ideally accept a range...

Copy link
Member

Choose a reason for hiding this comment

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

Definitely wouldn't hurt from a convenience POV, but it stores a string inside anyway, so that it couldn't eliminate the allocation.

Copy link
Member Author

Choose a reason for hiding this comment

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

or maybe some rename filename/last segment? (or any segment even)

Copy link
Contributor

Choose a reason for hiding this comment

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

so that it couldn't eliminate the allocation.

Hmm, well I guess it could be templatized (would be nice with the hopefully soon upcoming RCString), but it's a different story entirely.
Anyhow I opened an issue, s.t. it's not totally forgotten (vibe-d/vibe.d#2119).

throw new Exception("Failed to generate logo");
}

void unsetPackageLogo(string pack_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW this is typically called delete (though I don't want to get into this nitpicking)

redirect("/my_packages/"~_packname);
}

@auth @path("/my_packages/:packname/set_logo")
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW this is usually mapped on /logo and only the http method is used to distinguish between get, post and delete.

Copy link
Member Author

Choose a reason for hiding this comment

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

well all other endpoints are like this too

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's keep that for another day. The current scheme is just used to avoid HTTP method override hacks to account for the form based nature of the interface.

Copy link
Member

@s-ludwig s-ludwig left a comment

Choose a reason for hiding this comment

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

@wilzbach Anything left from your side?

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

LGTM too - though SVG upload ends in an non-terminating request for me. Maybe we should simply disallow svg for now and only accept png + jpg?

]]);
}
else
m_packages.update(["name": packname], ["$unset": ["logo": 1, "logoHash": 1]]);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's quite untypical to use 1 as null value.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is $unset, not $set

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes and I don't mind that you use 1.
It's just more common to use 0 or -1

(Of course that mongodb itself promotes such a crap API is a different story)

Copy link
Member Author

Choose a reason for hiding this comment

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

hm just took the first result off google when searching for how to unset

auto hash = bpack.tryIndex("logoHash");
if (!hash.isNull)
rev = hash.get.get!BsonBinData.rawData;
return logo.get.get!BsonBinData.rawData;
Copy link
Contributor

@wilzbach wilzbach Mar 18, 2018

Choose a reason for hiding this comment

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

It would be really to cool to have something like safeDeref for Mongo's data, it could then be sth. like:

rev = bpack.try.logoHash.rawData.get!BsonBinData;
return bpack.try.logo.rawData.get!BsonBinData;

h3 Logo
- string logoPath = "/packages/" ~ packageName ~ "/logo";
img.packageLogo(src=logoPath)
input(name="logo", type="file", accept="image/*", onchange="updateLogoPreview(this)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe accept="image/x-png,image/jpeg" or accept=".png, .jpg, .jpeg" for now?

enforceUserPackage(_user, _packname);
const FilePart logo = request.files.get("logo");
enforceBadRequest(logo != FilePart.init);
auto renamed = NativePath.fromString(logo.tempPath.toString ~ logo.filename.name.extension);
Copy link
Contributor

Choose a reason for hiding this comment

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

Check file extensions here?

{
static assert (isWeaklyIsolated!(typeof(&generateLogoUnsafe)));
static assert (isWeaklyIsolated!NativePath);
return (() @trusted => async(&generateLogoUnsafe, file).getResult())();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should to timeout after X seconds and terminate the convert process (at the moment it doesn't)

@WebFreak001
Copy link
Member Author

@wilzbach the issue with the hanging was actually an issue because of the code, fixed it now and svgs should work now, but because now we are restricting to png, bmp, jpg and gif it wont

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the infinite upload loop. I left a few more nits (some of them are really just comments)

if (f.size > 1024 * 1024)
document.querySelector(".logoError").textContent += "Error: Image file size too large! ";
if (f.type != "image/png" && f.type != "image/gif" && f.type != "image/jpeg" && f.type != "image/bmp")
document.querySelector(".logoError").textContent += "Warning: Invalid image type. ";
Copy link
Contributor

Choose a reason for hiding this comment

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

On uploading an invalid image - I see the following:

image

Idea: reorder these two messages to check for the image type first and then immediately return the function if the user used an invalid image type.
This also prevents them from hitting upload


auto sizeInfo = execute(["identify", "-format", "%w %h %m", firstFrame]);
if (sizeInfo.status != 0)
return new LogoGenerateResponse(null, "Malformed image.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this what exceptions are for? (I know that async / Future doesn't really support this at the moment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would throw here, but it was broken when I tried

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm aware - Future doesn't really support exceptions like es6 promises.

Copy link
Member

Choose a reason for hiding this comment

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

And it also can't, at least in a safe way, because of their thread-local nature in D, especially since the latest RC additions. It should require the callback to be nothrow, though.

}

// need to return * here because stack returned values get destroyed for some reason...
private LogoGenerateResponse* generateLogoUnsafe(NativePath file) @safe
Copy link
Contributor

Choose a reason for hiding this comment

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

That's weird (and looks like a bug in async). We should probably look into reducing and fixing it. A simple example works:

/+dub.sdl:
dependency "vibe-core" version="~>1.4"
+/
import vibe.core.core;
import vibe.core.concurrency;
import vibe.core.log;

import core.time;

void main()
{
    struct Foo {
        int r;
        string k;
    }
	auto val = async({
	    10.msecs.sleep;
		auto f = Foo(2, "aa");
		return f;
	});
	2.msecs.sleep;
	logInfo("Result: %s", val.getResult());
	runApplication;
}

img.packageLogo(src=logoPath)
p.logoError.error
label
| Select new Logo (at most 1 MiB, optimal size 512x512)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: typically max. is used for this (or not displayed at all except for the error case)

timer.stop();
if (result != 0)
{
if (!timer.pending)
Copy link
Contributor

Choose a reason for hiding this comment

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

You manually stop the timer before, so this can never happen.
While this shouldn't be a problem anymore now that only a few image formats and sizes are allowed, ideally such timing code should be put in utils, s.t. it's easier to abstract / test / reuse it (or part of vibe-core).
For now the timeout from coreutils might be simpler and less error-prone to use?

Copy link
Contributor

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

I'm really running out of points here ;-)
Imho we should merge this. There are still a few small points, but they can be done after to this PR (e.g. the upload button could be set to disabled by default and only enabled when it can be used)

if (!f) return;
document.querySelector(".logoError").textContent = "";
if (f.type != "image/png" && f.type != "image/gif" && f.type != "image/jpeg" && f.type != "image/bmp")
document.querySelector(".logoError").textContent += "Warning: Invalid image type. ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I meant to use return to avoid creating the object url and displaying something that's invalid.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used this because it signals the user that the image is now broken instead of showing the valid previous image.

@s-ludwig
Copy link
Member

So, let's pull the trigger!

BTW, I made some package graphics back when designing the new DUB logo. The logo on the left side is not right, but this could be adjusted to serve as a more refined package logo later on:
dub-package

@s-ludwig s-ludwig merged commit e9411cb into dlang:master Mar 21, 2018
@WebFreak001
Copy link
Member Author

WebFreak001 commented Mar 21, 2018

yeah that looks pretty cool, but I can imagine that it won't look good as small icon. I designed my icon so that it still looks good and recognizable even at 16x16. Though for large icons that could be cool. Maybe use the other D logo now that they are considering to rebrand to for the right side? Also I would remove or redo the shadows at the bottom

Compare: Image showing both dub logos as favicon

@s-ludwig
Copy link
Member

I don't think we should ever display the project logo that small, because who knows how the custom ones look like. But due to the coarse red strips it actually looks pretty decent in tiny sizes: dub-package-16 (the "D" could be made brighter at that size to stay readable)

True with the shadows. they need a frame or more space to the bottom to work like this. I missed the rebranding topic, do you have a reference?

@WebFreak001
Copy link
Member Author

uh I think I asked in #offtopic on slack once if the D logo there on https://github.com/dlang-community/artwork is the new D logo and they are planning on doing this, though it still has problems with small sizes like icons.

@s-ludwig
Copy link
Member

Ah okay, I see. My feelings about using it as the language logo are a little bit mixed, although it definitely is an awesome logo in general. Just as a side note, it could also make sense to go for a more gentle redesign, such as separating the moons and adjusting the shape of the "D". For example regarding the moons:

d-logo

@MartinNowak
Copy link
Member

Nice to see this merged :).

I'd still say we should seed all package logos with the homepage/repo's og:image instead of adding a default logo. It simply reduces the necessary initial configuration for package maintainers.

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.

5 participants