Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introducing chart tools manager including new crosshair tool #3466

Closed
wants to merge 2 commits into from

Conversation

OnkelDok
Copy link
Member

@OnkelDok OnkelDok commented Jul 26, 2023

Here my suggestion for a chart tool manager which provides the existing measurement tool and a new crosshair tool.
If you have any ideas or suggestion for different and maybe better implementation of such a chart tool management (ChartToolsManager), then please share your thoughts.
One thing I want to change the next days is the size of the crosshair in the button image. I think it can be a little bit bigger. done

https://forum.portfolio-performance.info/t/fadenkreuz-in-charts/7335

And here a short video:

2023-07-26.22-42-06.mp4

Good night!

@Nirus2000
Copy link
Member

Another masterpiece... 💯
If you have time, would you email me?

@OnkelDok
Copy link
Member Author

@Nirus2000: how can I contact you? I cannot find any way to send you private message in the PP forum or at github. But Andreas sent me one some days ago in the forum. Did I miss something?
Are you able to send me private messages in the PP forum?

Btw: I enlarged the images for the crosshair buttons.

@Nirus2000
Copy link
Member

Write on... webmaster ääääätttt nirus-online.de

@Nirus2000
Copy link
Member

Nirus2000 commented Jul 29, 2023

Maybe it is better to change the button labels according to the Contributing rules.

BtnLabel... --> see here

Messages.LabelCrosshair --> Messages.BtnLabelCrosshair
Messages.LabelMeasureDistance --> Messages.BtnLabelMeasureDistance

@buchen
Copy link
Member

buchen commented Jul 31, 2023

About:

Messages.LabelCrosshair --> Messages.BtnLabelCrosshair

I wasn't aware of this "guideline" in the contributing readme. I will change it.
While there are some labels that start with "BtnLabel", let's not add more.
Often the label can be used in various places - buttons, tooltips, columns, window titles - and then the "BtnLabel" ist just restricting the usage too much.

In general, my thinking is:

  • "Label" for plain labels, "Msg" for longer messages & texts, "Column" for column header
  • If an domain area is big enough (and specific enough) it might warrant its own domain-specific prefix (say CSV, PDF, Preferences, Chart, ...)
  • When in Rome, do as the Romans do - if there is a domain-specific prefix use it, otherwise do not introduce new domains

@OnkelDok - do not changes anything right away - I'll need some more time to look at the code

Copy link
Member

@buchen buchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @OnkelDok,

looks good and works good (I tested in on my machine - there it is smooth and fast).

I added some small comments and would appreciate if you can update your pull request.

Sorry for the back and forth on the naming of the labels. I will update the contributing guide.

Andreas.

@OnkelDok
Copy link
Member Author

@buchen, thank you for your feedback. I understood them all, but I will answer them and make changes when I have more time.
Maybe this evening, or tomorrow.

* reverted translation field names ("BtnLabel..." to "Label...")
* separate files for the tools
* fix text formating in crosshair tool
* some smaller non-functional refactoring
@OnkelDok OnkelDok requested a review from buchen July 31, 2023 19:18
buchen pushed a commit that referenced this pull request Aug 3, 2023
@buchen
Copy link
Member

buchen commented Aug 3, 2023

Merged 👍

Thanks @OnkelDok for the contribution. I love how we now have the tools manager handling all tools and at the same time have short and concise classes for the tools itself. That looks very maintainable.

@buchen buchen closed this Aug 3, 2023
@OnkelDok OnkelDok deleted the crosshair branch August 4, 2023 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants