Page MenuHomeFreeBSD
Differential D24966

lualoader: improve the design of the brand-/logo- mechanism
ClosedPublic
Actions

Authored by kevans on May 22 2020, 8:45 PM.
Tags
None
Referenced Files
F105812998: D24966.id72263.diff
Sat, Dec 21, 1:52 AM
Unknown Object (File)
Thu, Dec 5, 6:24 PM
Unknown Object (File)
Thu, Nov 28, 8:15 AM
Unknown Object (File)
Thu, Nov 21, 11:43 AM
Unknown Object (File)
Nov 19 2024, 7:55 AM
Unknown Object (File)
Nov 19 2024, 7:52 AM
Unknown Object (File)
Nov 19 2024, 4:33 AM
Unknown Object (File)
Nov 19 2024, 2:24 AM
View All 95 Files
Subscribers
imp

Details

Reviewers
freqlabs
Commits
rS366435: lualoader: improve the design of the brand-/logo- mechanism
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 out graphics types 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. The format has been renamed to 'gfx-*' and each one can provide a logo and a brand.

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kevans created this revision.May 22 2020, 8:45 PM
Herald added a subscriber: imp. · View Herald TranscriptMay 22 2020, 8:45 PM
kevans requested review of this revision.May 22 2020, 8:45 PM
Harbormaster completed remote builds in B31245: Diff 72142.May 22 2020, 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 .May 22 2020, 8:53 PM
freqlabs added a comment.EditedMay 22 2020, 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?).

kevans added a comment.May 22 2020, 10:50 PM
In D24966#549765, @freqlabs wrote:

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.May 26 2020, 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.

Harbormaster completed remote builds in B31304: Diff 72263.May 26 2020, 1:27 AM
freqlabs added inline comments.May 26 2020, 3:43 AM
stand/lua/logo-orbbw.lua
31 ↗(On Diff #72263)

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
kevans updated this revision to Diff 77654.Sep 29 2020, 9:42 PM
kevans edited the summary of this revision. (Show Details)

I lost the branch this was on- oops.

Improvement time! Return a table of graphics types mapped to definitions, have processFile() just generically process the file and iterate over what's been returned and setup as appropriate. All of the logo files have now been renamed to the agnostic gfx-* names and now return the new format, compatibility is maintained for the older ones to give downstreams time to adjust. I'll remove it after FreeBSD 13 branches.

Harbormaster completed remote builds in B33888: Diff 77654.Sep 29 2020, 9:42 PM
freqlabs added inline comments.Oct 4 2020, 8:53 PM
stand/lua/drawer.lua
71 ↗(On Diff #77654)

"Nothing returned" is not a reasonably descriptive error message.

85 ↗(On Diff #77654)

Odd to print the error message return true, instead of returning false and the error message as done above.

101 ↗(On Diff #77654)

It would be good to check for false and print the error.

122 ↗(On Diff #77654)

Likewise re: error handling

kevans updated this revision to Diff 77868.Oct 4 2020, 10:18 PM
kevans marked 2 inline comments as done.

Address error handling concerns (maybe)

Harbormaster completed remote builds in B33988: Diff 77868.Oct 4 2020, 10:18 PM
freqlabs accepted this revision.Oct 4 2020, 10:29 PM
This revision is now accepted and ready to land.Oct 4 2020, 10:29 PM
Closed by commit rS366435: lualoader: improve the design of the brand-/logo- mechanism (authored by kevans). · Explain WhyOct 4 2020, 10:42 PM
This revision was automatically updated to reflect the committed changes.
kevans added a commit: rS366435: lualoader: improve the design of the brand-/logo- mechanism.

Revision Contents
Changeset List

PathSize
head/
7 lines
stand/
lua/
10 lines
57 lines
55 lines
54 lines
49 lines
52 lines
51 lines
59 lines
58 lines
53 lines
56 lines
55 lines

Diff 77870

View Options

head/ObsoleteFiles.inc

Loading...
View Options

head/stand/lua/Makefile

Loading...
View Options

head/stand/lua/drawer.lua

Loading...
View Options

head/stand/lua/gfx-beastie.lua

Loading...
View Options

head/stand/lua/gfx-beastiebw.lua

Loading...
View Options

head/stand/lua/gfx-fbsdbw.lua

Loading...
View Options

head/stand/lua/gfx-orb.lua

Loading...
View Options

head/stand/lua/gfx-orbbw.lua

Loading...
View Options

head/stand/lua/logo-beastie.lua

Loading...
View Options

head/stand/lua/logo-beastiebw.lua

Loading...
View Options

head/stand/lua/logo-fbsdbw.lua

Loading...
View Options

head/stand/lua/logo-orb.lua

Loading...
View Options

head/stand/lua/logo-orbbw.lua

Loading...