Opened 12 years ago

Closed 11 years ago

#251 closed enhancement (fixed)

Colour is a presentational attribute, but it's set in many different places

Reported by: nhemingway Owned by: djmitche
Priority: minor Milestone: 0.7.10
Version: 0.7.6 Keywords: review
Cc: nhemingway

Description

The colour used to present steps is set in many different places, most of which have no other presentational function. Standard practice is to separate presentation from function.

The waterfall seems to not depend on the colour set for a step (relying only on CSS) The statuslog is text only so colour is irrelevant (?) The statusgui does seem to depend on the step's colour, but that could be set more simply based on the step's result The debugclient seems to have no function that would involve colour.

If I have any of this wrong (wildly wrong?), pls let me know.

I have created the attached (diff -u) patch, which seems to work for both of the above clients that use colour and passes all of the tests.

Attachments (1)

buildbot_colors.diff (30.1 KB) - added by nhemingway 12 years ago.
darcs diff -u removing color from all but presentation code

Download all attachments as: .zip

Change History (10)

Changed 12 years ago by nhemingway

darcs diff -u removing color from all but presentation code

comment:1 Changed 12 years ago by warner

  • Milestone changed from undecided to 0.7.8
  • Owner changed from nhemingway to warner
  • Status changed from new to assigned

comment:2 Changed 11 years ago by dustin

  • Milestone changed from 0.8.0 to 0.7.+

comment:3 Changed 11 years ago by dustin

  • Milestone changed from 0.7.+ to 0.7.10

Let's see if this works with the current code; if so, I'll get it into 0.7.10.

comment:4 Changed 11 years ago by dustin

Hmm, no, I think this patch is just a bit dated. Neil, how important do you consider this separation, given that the whole thing will be reworked (with some thought toward separation of presentation from process) in one-oh?

comment:5 Changed 11 years ago by dustin

sorry, this is in my 'bug251' branch: http://github.com/djmitche/buildbot/tree/bug251

comment:6 Changed 11 years ago by nhemingway

I'm now ambivalent, but for the fact that (as at work, where a colleague has deleted 25% of the codebase with no reduction in functionality), you shouldn't have to look through spurious code to understand what the system is doing

comment:7 Changed 11 years ago by dustin

  • Milestone changed from 0.7.10 to 0.7.+

OK, well, I won't stress myself out to update the patch, then.

comment:8 Changed 11 years ago by nhemingway

  • Keywords review added
  • Owner changed from warner to djmitche
  • Status changed from assigned to new

comment:9 Changed 11 years ago by dustin

  • Milestone changed from 0.7.+ to 0.7.10
  • Resolution set to fixed
  • Status changed from new to closed

great, thanks!

commit cdefef4eebd45cf6bc17b58f605cb24987ab619d
Merge: 620c248... d1190bd...
Author: Dustin J. Mitchell <dustin@zmanda.com>
Date:   Wed Feb 18 16:26:26 2009 -0500

    Merge branch 'bug251' of git://github.com/nhemingway/buildbot
Note: See TracTickets for help on using tickets.