Page MenuHomeFreeBSD

lualoader: improve the design of the brand-/logo- mechanism
Needs ReviewPublic

Authored by kevans on Fri, May 22, 8:45 PM.

Details

Reviewers
freqlabs
Summary

In the previous world order, any brand/logo was forced to pull in the drawer and call drawer.add{Brand,Logo} with the name their brand/logo is taking and a table describing it.

In the new world order, these files just need to return a table that maps logo/brand names to a table of the exact same format as what was previously being passed back into the drawer. The appeal here is not needing to grab a reference back to the drawer module and having a cleaner data-driven looking format for these.

drawer.addBrand/drawer.addLogo will likely remain in place for the foreseeable future, as there's no overhead to them and it's not worth the break in compatibility with any pre-existing brands and logos.

A future improvement may be some mechanism by which we can denote groups of brands/logos instead, e.g. don't allow - in their names and take the first part of the name as the filename, so one file named logo-freebsd.lua could provide, for example: freebsd-base and freebsd-basebw logos by exporting base and basebw.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 31304

Event Timeline

kevans created this revision.Fri, May 22, 8:45 PM
kevans requested review of this revision.Fri, May 22, 8:45 PM
kevans retitled this revision from lualoader: imrpove the design of the brand-/logo- mechanism to lualoader: improve the design of the brand-/logo- mechanism .Fri, May 22, 8:53 PM
freqlabs added a comment.EditedFri, May 22, 10:15 PM

I like the direction toward the artwork being proper Lua modules, but I think to be more idiomatic we should simply return the table for the module, rather than exporting it as a name. This should simplify loading a bit (should be possible to use try_include?).

I like the direction toward the artwork being proper Lua modules, but I think to be more idiomatic we should simply return the table for the module, rather than exporting it as a name. This should simplify loading a bit (should be possible to use try_include?).

Sure- I had avoided this because the original author of these files for some unknown reason made them all return true...but this is lua and I can just check the type. It's incredibly unlikely anyone's written something otherwise incompatible with that.

kevans updated this revision to Diff 72263.Tue, May 26, 1:27 AM
kevans edited the summary of this revision. (Show Details)

Simplify even further, just return the table. Consolidate a little bit and just pass in the addLogo/addBrand functions to a single processor that does both, since the interfaces are equivalent.

freqlabs added inline comments.Tue, May 26, 3:43 AM
stand/lua/logo-orbbw.lua
31

If you want to eventually combine logo and brand into one module, it might be a better to have this be brand or logo as appropriate, instead of orbbw (which is just duplicating information we already have in the module name). Then an artwork module would look like this:

return {
    brand = {
        graphic = {"cool ascii brand art",}
        shift = {x = 0, y = 0},
    },
    logo = {
        graphic = {"cool ascii logo art",},
        shift = {x = 4, y = 2},
    },
}

And loading might go something like this:

local function loadArtwork(filename)
	if name == nil then
		return false, "Missing name"
	end

	-- This would probably need to try a few different filenames
	-- for legacy compatibility.
	local filename = name .. '.lua'
	local ret = try_include(filename)
	if ret == nil then
		return false, "Nothing returned"
	end

	-- Legacy format
	if type(ret) ~= "table" then
		return true
	end

	if ret.brand ~= nil then
		drawer.addBrand(name, ret.brand)
	end
	if ret.logo ~= nil then
		draw.addLogo(name, ret.logo)
	end
end