fix: rework boss bar feature #6
Conversation
drolpi
left a comment
drolpi
left a comment
There was a problem hiding this comment.
In general, I think the implementation is very good. I have noted some small changes, some of which you couldn't know, which I sometimes don't know exactly myself haha. Thanks for the implementation :).
| protected void start(Counter counter) { | ||
| super.start(counter); | ||
| if (!this.bossBarFeature.isRegistered(replaceBossBarId)) { | ||
| this.bossBarFeature.register(this.replaceBossBarId, this.color, this.overlay); |
There was a problem hiding this comment.
I don't think we should register a boss bar during the runtime of a phase. If at all, this should happen in the constructor, because it is called during the configuration of a phase / game or directly after loading the configuration file
| this.bossBar.setProgress(progress); | ||
| protected void set(Player player, long count, float progress) { | ||
| if (!this.bossBarFeature.isRegistered(replaceBossBarId)) { | ||
| this.bossBarFeature.register(this.replaceBossBarId, this.color, this.overlay); |
There was a problem hiding this comment.
Same as above, here too we should not register during the term of a phase
| //TODO: Lang | ||
| this.bossBar.setTitle("" + count); | ||
| this.bossBar.setProgress(progress); | ||
| protected void set(Player player, long count, float progress) { |
There was a problem hiding this comment.
The player is not used, is it?
There was a problem hiding this comment.
Maybe we should change the AbstractCounterProgressFeature so that there is no player loop and move it to the LevelCounterFeature, because it is not needed for the BossBarCounterFeature and the method is called unnecessarily often.
| private BarColor color = BarColor.BLUE; | ||
| @Expose | ||
| private BarStyle style = BarStyle.SEGMENTED_20; | ||
| private final List<BossBarInfo> bossBarInfos; |
There was a problem hiding this comment.
I would rename the field to "bossBars" as it is correctly named in the Java code but not relevant in the config for the end user. My preferred alternative is to use the @SerialiedName annotation from Gson. https://www.javadoc.io/doc/com.google.code.gson/gson/2.6.2/com/google/gson/annotations/SerializedName.html
|
|
||
| private final Map<String, BossBarContainer> bossBars; | ||
|
|
||
| private final Phase phase; |
There was a problem hiding this comment.
I would declare the phase directly under the game, since our field sorting is unofficially always like this:
- Attributes that are passed by the constructor or are related to a passing class.
- Attributes that are exposed and thus stored in the config
- Non-final attributes that are changed during (in this case feature runtime).
This should also be observed in other classes.
|
|
||
| private BossBar bossBar; | ||
| @Expose | ||
| private final String replaceBossBarId = "counter"; |
There was a problem hiding this comment.
Maybe rename?, a little bit long and redundant.
| private BossBar bossBar; | ||
| @Expose | ||
| private final String replaceBossBarId = "counter"; | ||
| private final Component component; |
| return this.getContainer(id).hidden; | ||
| } | ||
|
|
||
| public void setHidden(String id, boolean hidden) { |
There was a problem hiding this comment.
We dont use set or get prefixes.
| return this.getContainer(id).bossBar.progress(); | ||
| } | ||
|
|
||
| public void setProgress(String id, float progress) { |
There was a problem hiding this comment.
We dont use set or get prefixes.
|
|
||
| public BarColor color() { | ||
| return this.color; | ||
| public void setName(String id, Component name) { |
There was a problem hiding this comment.
Should also contain the tag resolvers, as these are directly related to the text.
No description provided.