Handle text_alignment in Milestone

[kajuberdut]

This change addresses issue #75 by shifting text 1/2 it's length left or right when "left"/"right" are passed as text_alignment.

[kajuberdut]

This is a very simple version of aligning Milestone text. I think a better version would be to allow pixels or % as additional arguments like text_alignment="left:100" for 100 pixels to the left or text_alignment="left:50%" for 50% of text length to the left. If this sounds good I can update my pull request to implement these.

[felix-seifert]

I know too little about the implementation of Roadmapper's functionality, and I highly doubt that we have a useful example for unit tests. However, I highly recommend that when you modify a feature, you write a simple unit test that ensures this functionality works as expected. Besides explaining your actual code changes, such tests ensure that your functionality does not regress with future changes.

[kajuberdut]

I know too little about the implementation of Roadmapper's functionality, and I highly doubt that we have a useful example for unit tests. However, I highly recommend that when you modify a feature, you write a simple unit test that ensures this functionality works as expected. Besides explaining your actual code changes, such tests ensure that your functionality does not regress with future changes.

I agree that in general unit tests are a good idea but in this case I haven't any idea how to make a useful assertion about this feature and don't want to write a meaningless test for the sake of having a test. If you can suggest a way to assert that a text has been offset I am happy to write a test.

[kajuberdut]

``> This is a very simple version of aligning Milestone text. I think a better version would be to allow pixels or % as additional arguments like text_alignment="left:100" for 100 pixels to the left or `text_alignment="left:50%"` for 50% of text length to the left. If this sounds good I can update my pull request to implement these.

For my own use I wanted the ability to do alignment by percent or pixel so I implemented it. If you wold rather see this code (or this code with additional requested changes) in the pull request please let me know:

text_width, _ = painter.get_text_dimension(text=self.text, font=self.font, font_size=self.font_size)

# Text is already "centered" if we change nothing
# For "left" and "right", move text_x by 1/2 text_width.
if ":" in self.text_alignment:
    alignment, modifier = self.text_alignment.split(":")
else:
    alignment = self.text_alignment
    modifier = None

if modifier is None:
    offset = .5 * text_width
elif "%" in modifier:
    offset = (int(modifier.strip("%")) / 100) * text_width
else:
    offset = int(modifier)

if alignment == "right":
    self.text_x = self.text_x + offset
elif alignment == "left":
    self.text_x = self.text_x - offset

[felix-seifert]

I had a look at your edited class and method. Testing this method is indeed slightly weird as the method does not really have an output but generates a file.

My suggestion is to create an integration test for this. We already have quite generic tests which basically compare the generated roadmaps with how the roadmap should look.

Your task would be to create the code for a roadmap similar to ColourTheme. Then, add the class of your roadmap to the same lists as ColourTheme and ColourThemeExtensive. Please pay attention to giving appropriate names.

After you commited your changes, we can help you to generate the example roadmaps on the GitHub servers. Alternatively, you execute the workflow to generate the roadmaps from your personal account.

[csgoh]

Hi,

Sorry for the late response, as I am currently on holiday.

Thanks for the feedback and improvement suggestions. I really appreciate it.

@kajuberdut It is indeed a very simple text alignment implementation. I looked at your suggestion, and I think it is a good idea. However, as @felix-seifert was saying, it would be great if you could write the corresponding test cases. To be honest, I am pretty bad at writing test cases and would definitely need some help.

CS

[kajuberdut]

I'll implement the test case over the coming weekend. Thanks!

[kajuberdut]

Ok, as of today I have re-worked it to allow the options I mentioned above like "left:100" for 100 units left, or "left:75%" for 75% of text width to the left. This change means there is some logic for unit tests so I also added unit tests in: src/tests/test_milstone.py

I also added a src/tests/roadmap_generators/milestone_text_alignment.py to generate a roadmap with examples of the text alignment in milsetones and added it to the same lists that ColourTheme was in.

The unit tests are all passing:

@felix-seifert I will now look into running the workflows or you are welcome to jump in on those. Thank you.

[kajuberdut]

Ok, it took a few iterations but I have both workflows passing in my fork: https://github.com/kajuberdut/roadmapper/actions

@felix-seifert or @csgoh if either of you could kick off the workflows when convenient or let me know if I need to do something else first that would be great. Thanks.

[felix-seifert]

I love that you add test cases. ❤️ Especially that you also discovered this opportunity for unit tests. ❤️ In general, I like that you contribute to roadmapper and want to improve it.

The comments I left are not really about missing functionality but more about the code readability. Theoretically, we could just merge your changes. However, we should consider the long-term quality of roadmapper and embrace the possibility of learning.

[felix-seifert]

I do not really know which syntax should be used on the user's side.

• In the code, you can consider adding a comment with a simple example.
• Our documentation should get a nice description of what users can do.

[kajuberdut]

Ok. This seems to be the last comment standing however:

1. There do not seem to be code examples in the code anywhere else, there are for the most part no docstrings, so it feels a bit odd to add a docstring with a code example.
2. On the other hand, I can't see any way to include an update to the wiki in this pull request.

I think if this pull request is satisfactory, the next step would be to update the other objects that allow alignment to use the new Alignment class and to offer % and unit offset options. Then the wiki should be updated with a description of that feature.

[kajuberdut]

In my update just now I refactored alignment out to it's own class so that these offset by units / percent etc options can be easily used in other items that have alignment (however, percent is a bit of a problem since in other cases it is the percent of the bounding object instead of the percent of the text.)

I have added unit tests for the new class and updated Milestone to use the Alignment class to handle most of the logic for alignment. However, I have failing tests and also need to address @felix-seifert's comment about updating the documentation so this is not ready for review again. Also I have run out of weekend so this might have to wait a week for me to complete it. Thanks for the useful feedback so far.

[kajuberdut]

I had some time so I fixed the bug I had introduced that was failing unit tests and I also added a validation to avoid the impossible case of center + offset. I think this is ready to review and all tests are passing.