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

[will be dropped]From my own purpose. #32

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Robert-Ordis
Copy link
Contributor

All of these changes are came from my purpose (making real-time rewindable plotter app.).

  • Able to seek/rewind timeline.
  • Able to remove Serie from Chart.
  • Avoid to be affected by NTP
  • Able to push arbitrary ordered timeline data.

If you are interested in these code or thought you can make these code better,
then take a light look please.

@Robert-Ordis
Copy link
Contributor Author

Robert-Ordis commented Mar 7, 2022

sorry i forgot to commit "changed" test code.
I'll push them later.

->pushed

@lcallarec
Copy link
Owner

Whooo, very interesting ! Thanks for sharing, I'll take a deeper look in the next weeks, but I like what you've done.

@Robert-Ordis
Copy link
Contributor Author

Thank you for replying.
Because of prefering to implement earlier, there are various dirty code probably.
So, please show me the better approach if you find.(especially, Config.TimeSeek, Config.width...)

@Robert-Ordis
Copy link
Contributor Author

Can I submit more pull requests ?
I tried to add more abilities for my own purpose.

  • Showing reticle and actual value from specified position(x, y)
  • Handling mouse event: to scroll and click legend area to detect the legend that user want to do something.
  • Handling mouse event: pointing a plot area to manipulate a reticle.
  • fix: 0 < y < 1 at first will make app crush (at least my environment)
  • Showing minus value.
  • and some tweaks.

But more abilities are implemented on the original repository after this pull request.
So, I don't understand if I can push these modifications.

vala_args = ['--target-glib=2.50']

#setting up gtk
gtk_major = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

GTK version should define in meson_options.txt:

option('GTK',
       type: 'integer',
       value: 4,
       description : 'GTK version')

then use it in the meson:

if 3 == get_option('GTK')
else
endif

then change the GTK version by command:
meson -DGTK=3 build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
I tried it, and it works.
I'll take it in my repository later.

Copy link
Owner

Choose a reason for hiding this comment

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

By the way, as talked in #37, the default build should be GTK3 for now if we don't provide any argument to meson.

@taozuhong
Copy link
Contributor

Do you mind that your repo behind upstream 20 commits?

you should rebase to upstream immediately.

@Robert-Ordis
Copy link
Contributor Author

Robert-Ordis commented Nov 29, 2022

Yes, I know.
I'm wondering if my pull request will be accepted or rejected, ... or just ignored or forgotten.
(I knew Mr.Callarec's newer commiting recently.)

Rebasing is my job for now.
But on the other hand, I'm thinking that if my PR would be left alone, then I have to work for my version as the branched version.

From the very start, my modification is for my own project which named "omni-plotter".

@stsdc
Copy link
Contributor

stsdc commented Nov 29, 2022

hey, @lcallarec can you look into this PR? Would be cool to have these features 😄

@Robert-Ordis
Copy link
Contributor Author

@stsdc Thank you for your comment.
Did you see my code?
2nd of my additional features(Mainly, about the reticle) are not in "master" branch in my repository.

@taozuhong
Copy link
Contributor

taozuhong commented Nov 29, 2022

Do you mind rebasing my PR #37? it's done for GTK4 now, all examples work fine.

@Robert-Ordis
Copy link
Contributor Author

@taozuhong
Thx. I'll find my time to do it.
I also tried to performe my code on GTK4(may be working).
(I really hate the breaking change of API of 3 to 4...lol)

@lcallarec
Copy link
Owner

Hey @Robert-Ordis, thank you very much for your PR. That's a fact : I don't have so much time, but yeah, you're welcome to create PR. Just : for each new feature, please update the documentation accordingly, add tests (TDD welcome), and avoid API breaks. If you need any API breaks, let discuss together.

If any of you wants to become a maintainer to help me pushing live-chart at the next level, let's talk, I'll be very proud !

By the way, I'll merge this PR soon, sorry for the delay, thank you again.

@Robert-Ordis
Copy link
Contributor Author

Robert-Ordis commented Nov 30, 2022

Good morning @lcallarec .
I'm glad to receive your reply. (sorry for my bad English if this is hard to read)
Because of my too much piled up modification, reviewing/merging will need a little longer time...(I'm sorry for it)

Avoiding API breaks: I did my work as not to lose compatibility (I think it means becoming unable to reuse "example" code e.g.) carefully at least. But changing the base type of Values (LinkedList->TreeSet) may be so.

Test and documentation: I see. I'll commit the example code for my features.
Does "documentation" mean README.md ?

I see that you'll merge my PR. thx, but then, do I have to rebase to your newest repository?

@taozuhong
Copy link
Contributor

do I have to rebase to your newest repository?

Yes, and move forward to GTK4 at best.

@lcallarec
Copy link
Owner

lcallarec commented Nov 30, 2022

@Robert-Ordis : yes, in order to technically be able to merge your PR, you need to resolve conflicts, whatever the way you do it. You can fix them via the web editor (somewhere below this message, there should be a "This branch has conflicts that must be resolved" block where you'll be able to open the web editor).
Please ask if you need further help.

@lcallarec
Copy link
Owner

  • yes, by documentation I mean README.md and code / feature examples in examples folder.

If you want to contribute with more PR in the future, I think it'll be a good idea to make a PR per feature. It's smaller, simpler to read and review and therefore will be easier and quicker to merge.

chart.remove_serie(rss);
return false;
});

Copy link
Owner

Choose a reason for hiding this comment

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

Will be a nice if we could have a button to remove that serie instead of waiting 20s

public void remove_all_series(){
this.series.remove_all();
}

Copy link
Owner

Choose a reason for hiding this comment

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

That kind of API improvements should be added to README.

this.prev_time = now;
this.queue_draw();
return true;
});
Copy link
Owner

Choose a reason for hiding this comment

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

Documentation is welcome :)

}

private bool render(Gtk.Widget _, Context ctx) {

ctx.set_antialias(Cairo.Antialias.NONE);
Copy link
Owner

Choose a reason for hiding this comment

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

Don't we loose render quality if we disable antialias ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry to forgot to revert it. It's my mistake.

set{
if(_width != value){
//i = config.width - config.padding.right; i > config.padding.left; i -= config.x_axis.tick_length
if(x_axis.tick_length <= 0.0){
Copy link
Owner

Choose a reason for hiding this comment

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

If it's commented, it's certainly useless :)

foreach(var entry in signals){
entry.key.disconnect(entry.value);
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Yes, very cool, you fixed lots of possible memory leaks 👍

@Robert-Ordis
Copy link
Contributor Author

Robert-Ordis commented Nov 30, 2022

Thanks for revieweing my noob-like PR...
Then, I'd like to make my modifications tidy up once. (Including the features not pushed here yet)
In this case, what should I do to my mods for Gtk4 ?
(#37 is still waiting for being merged)

P.S.
I'd like to keep this PR till next PR.

@Robert-Ordis
Copy link
Contributor Author

Started first (#38).

@taozuhong
How do I treat my mods for Gtk4 ?(maybe conflicting between mine and yours)

@taozuhong
Copy link
Contributor

Put your mods based on GTK4 if your app has been ported to GTK4, or focus on GTK3 full time, then upgrade to GTK4 branch.

@Robert-Ordis Robert-Ordis changed the title From my own purpose. [will be dropped]From my own purpose. Dec 1, 2022
Caution of memory leak.
@Robert-Ordis
Copy link
Contributor Author

Started 2nd (#39).

@Robert-Ordis
Copy link
Contributor Author

Started 3/10 (#40)

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.

4 participants