Forum > Lazarus Extra Components
FPVectorial merge request
Hansi:
Hi!
I've added a little update to FPVectorial and created the GitLab merge request
https://gitlab.com/freepascal.org/lazarus/lazarus/-/merge_requests/131
Unfortunately I don't know if everything is alright, or if I still have to do something. The CI/CD pipeline didn't succeed (even after I added my credit card number :o ). Will the merge request still be seen by a Lazarus maintainer and merged?
Please let me know what to do next or any other comment.
Thanks
Hansi
Bart:
The pipeline failure seems to have nothig to do with your code changes.
Bart
wp:
I'm not a git specialist, but I suspect that, since you are not a team member, you do not have the permission to merge your changes to the main branch directly. Write a bug report, and a team member will review your changes and merge them. Your credit card is not needed to participate in the development. ;D
Briefly looked over your changes. Having the font size as a floating point number suddenly, I guess this will this break existing code. Moreover there is the issue that the float-to-string conversions can create strings without point as decimal separator. The odt writer, for example, has a special function FloatToODTText for correct conversion to text, or if you prefer usage of the Format function, it contains a dedicated formatsettings record, named FPointSeparator. I did not look at the other writers, I guess it's similar there.
Hansi:
Hi!
Thanks Bart and wp for your feedback!
--- Quote from: wp on December 30, 2022, 08:04:31 pm ---I'm not a git specialist, but I suspect that, since you are not a team member, you do not have the permission to merge your changes to the main branch directly. Write a bug report, and a team member will review your changes and merge them. Your credit card is not needed to participate in the development. ;D
--- End quote ---
Yes, but is the existence of a merge request alone sufficient, or do I have to create a GitLab issue? Or even poke a maintainer directly?
--- Quote from: wp on December 30, 2022, 08:04:31 pm ---Briefly looked over your changes. Having the font size as a floating point number suddenly, I guess this will this break existing code. Moreover there is the issue that the float-to-string conversions can create strings without point as decimal separator. The odt writer, for example, has a special function FloatToODTText for correct conversion to text, or if you prefer usage of the Format function, it contains a dedicated formatsettings record, named FPointSeparator. I did not look at the other writers, I guess it's similar there.
--- End quote ---
Regarding existing code: Assigning an integer to a double is ok because of auto conversion (e.g., when calling AddText or setting a member variable). IMHO, the only interesting case is when a programmer reads back the font size from TvText.Font.Size. I guess this should be mentioned in the release notes.
Regarding decimal separator: Thanks for the hint, I have missed that detail! Fortunately, even on my German setup everything worked fine. But I'll rework the changes and update the merge request accordingly.
On a side note: It seems that the repository doesn't contain all files, because I couldn't find a single user of GenerateDebugTree, despite it being a very elaborate infrastructure behind. Perhaps also some self-tests exist (more than the examples).
Thanks
Hansi
Bogen85:
--- Quote from: Hansi on December 30, 2022, 08:55:01 pm ---Yes, but is the existence of a merge request alone sufficient, or do I have to create a GitLab issue? Or even poke a maintainer directly?
--- End quote ---
You should always create a GitLab issue for things like this.
The PR would be linked to in the initial issue report as a proposal for how you feel the issue could be resolved.
Typically the Lazarus forum or contacting developers directly is not the preferred (as it is usually impractical) method for change related item discussions like this.
The GitLab issue tracker is.
But of course not everyone can automatically be expected to be familiar with the process, and asking here is good!
Navigation
[0] Message Index
[#] Next page