Skip to content

features for timeline.#38

Merged
lcallarec merged 18 commits intolcallarec:masterfrom
Robert-Ordis:feature-timeseek
Dec 2, 2022
Merged

features for timeline.#38
lcallarec merged 18 commits intolcallarec:masterfrom
Robert-Ordis:feature-timeseek

Conversation

@Robert-Ordis
Copy link
Contributor

First, I'm sorry for committing 3 changes in this one PR.
This has changes about manipulating the timeline.

  • Seeking on the timeline
  • Pausing the timeline (but chart will be refreshed)
  • timeline formatting: be able to print the fractional part→Configuring on x_axis may be better rather than this.

 - Config.time.current = [int64 val] to specify the time.
 - Default unit of time is "m" (means [msec])
 - Drawbacks: Never fits to Serie.add(double value) when unit is "s"[sec] or "u" [usec]
 - tick_interval is [sec] -> then, config.time.conv_sec should be used.(was conv_msec)
 - If true, time on x_grid is shown to fractional part.
...

... and I forgot to convert tab to space4.
 - Shows speed ratio(*sec) while scrolling.
 - If play_ratio = 0.0, then it indicates to pause.
@Robert-Ordis
Copy link
Contributor Author

@lcallarec
That's all of mods on this part.
I'll submit the next PR after getting the disicion for this.

@lcallarec
Copy link
Owner

I'm glad to see this PR, kudos @Robert-Ordis ^^

Technically, it looks good. I will test very soon this feature on a real environment.

If everything is ok on a functional point of view, outside my small comment, I will merge this PR.

If you got time to improve this code in the future, I'll suggest you to :

  • Add test for TimeSeek class , especially the get_time_str method; if you're not confortable with testing, I can mentor you ;
  • Hide behind an abstraction how the timeseek is working : for exemple, instead of chart.config.time.current = GLib.get_real_time() / chart.config.time.conv_ms;, it will be better to have, for example :
chart.config.time.now();
  • IMHA, public properties conv_us, conv_ms, conv_sec should stay hidden from the oustide.

Anyway, that not blocking at all.

Thank you !

@Robert-Ordis
Copy link
Contributor Author

Robert-Ordis commented Dec 1, 2022

Dear, @lcallarec
Thank you for your reviewing.

I'm so happy for your evaluation...lol

Yes, your pointing about abstraction is right.
Excuse me to this, my app is using timeseek heavily from seekbar, then specifying actual timepoint is needed.

OK, I'll try to make a testcode for it. thank you so much.

@Robert-Ordis
Copy link
Contributor Author

Excuse me to additional commit.
I found some problem in my code and fixed them.

Inside story: On my app(https://github.com/Robert-Ordis/omni-plotter), I'm thinking that treating time in microsec might be needed in the future.

@lcallarec
Copy link
Owner

I just tested your PR, it works like a charm :)

Please fix the documenation issue I noticed earlier and your PR will be merged (up to you to improve the basecode according to my previous recommendations in another PR later)

- Fix the mistake on "Seeking on the timeline."
- Add "Specify the range of the timeline."
@Robert-Ordis
Copy link
Contributor Author

Robert-Ordis commented Dec 1, 2022

I pushed documentation mod (mainly related to dropped property and config.time.set_range)

Please fix the documenation issue I noticed earlier

Please tell me what the issue you noticed is. your suggestions in this PR?

@lcallarec
Copy link
Owner

The (small) documentation issue is here : https://github.com/lcallarec/live-chart/pull/38/files/0cd13fdd75f63d05e8cad992b1b25fcd02aaa72c#diff-3a1bd79644f8f66a114a45997f889dcba1ca809b0bb65865d4d7117ddc3e023fR32

As a rule of thumb, if a property / attribute / method is public and is meant to be used by a developper outside the project, it should be documented.

The suggestions I was talking about : #38 (comment), but it could be done later, it's not blocking)

@Robert-Ordis
Copy link
Contributor Author

About config.time.show_fraction ?
I see. But give me a little time to modify code.
I thought it should be on config.x_axis because it changes a format of timepoint printed in x-axis.

 - Because it changes a format of timepoints on "X-Axis".
 - about "show_fraction", moving from "TimeSeek" to "XAxis".
 - Name of example is "example-usec", but prev was "sec" xo
About x_axis.show_fraction
@Robert-Ordis
Copy link
Contributor Author

I'm sorry for keeping changing public part...
By the way, I wrote it as x_axis setting.

@lcallarec
Copy link
Owner

Good to merge !
That's a great feature that will add lots of value to live-chart. Thanks @Robert-Ordis for this one !

@lcallarec lcallarec merged commit 0fa254e into lcallarec:master Dec 2, 2022
@Robert-Ordis
Copy link
Contributor Author

Thank you very much for your reviewing.
Then, I'll go on the next PR work after getting home.

There are still 9 works for comitting PR... lol

@lcallarec
Copy link
Owner

Don't forget, for your futur PR, to add appropriate tests. Please note that you should test the business logic, but also the rendering (i.e. what the user is seeing on the screen, have a look at https://github.com/lcallarec/live-chart/blob/master/tests/smooth_line_area.vala#L3).

@Robert-Ordis Robert-Ordis deleted the feature-timeseek branch December 2, 2022 14:12
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.

2 participants