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

add kagi chart #93

Merged
merged 21 commits into from
Feb 26, 2021
Merged

add kagi chart #93

merged 21 commits into from
Feb 26, 2021

Conversation

tarkah
Copy link
Owner

@tarkah tarkah commented Feb 24, 2021

@miraclx @alfredo-catalano The Kagi chart is basically complete. You can start to test it off this branch if you'd like.

I still need to add ability to define custom reversal amounts from config file and in the GUI.

SHIFT+< & SHIFT+> can be used to scroll the Kagi chart when it is wider than the terminal (usually only an issue on 5Y timeframes).

Let me know what you think!

still need to add ability to define custom reversal amounts from config
file and in the GUI
@alfredo-catalano
Copy link

alfredo-catalano commented Feb 24, 2021

Very nice, thank you!

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

Nice work! Although, shift+left & shift+right is captured by Konsole and Yakuake for switching tabs. Do you think we could perhaps use < (less than) and > (greater than)?

Or we could ship with these defaults, and people like me with conflicting keybindings could use #65 when it's ready.

EDIT: this works just fine for me in Alacritty.

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

I also love that scrolling functionality, it might be very relevant in #80 (comment):

Another special feature might be selecting custom time frames, so a ctrl+mouse drag would select a custom range on the x-axis within the current time frame and zoom into that as a custom time frame.

That can be further extended to support L-R scrolling when zoomed in to custom timeframes.


EDIT: just realized you (@tarkah) already made that comment in #80 (comment)

"Zoom" would be a cool phase 2 of this. I have scrolling reliably implemented now (from the Kagi chart) and I think thatd the hard part of doing a zoom.

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

Hmm.. that sucks its captured by such a common console. Maybe [ & ] would be sane defaults, or < & > like you said? Or I could have it capture both since I still like shift + arrows.

Edit: Yeah, I think I will go with < & > on top of SHIFT + <- / ->

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

That sounds reasonable, tbh I actually like shift+<-/-> too. I'll check if I can free up those bindings to actually be able to use them with tickrs.

Again, love it! and thanks @alfredo-catalano for the chart suggestion.

EDIT: I was able to disable those bindings in both terminals, so it works fine now.

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

I guess, (for now), redundancy in the default keybindings can be okay but maybe after #65 (customizable keybindings), we can drop the <+> and leave it up to the users to configure that if they have a conflict.

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

Agreed!

So for the customization piece of Kagi chart with specifying reversals.... I was thinking of either adding a block, similar to the toggle block, with a header that said like 'e' to edit and the fields to edit inside it. OR having 'e' edit in the toggle window and have it open a pane, similar to options pane, that has the fields you can edit. Then that can be expanded on way easier in the future for additional configuration options since its a big pane

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

OR having 'e' edit in the toggle window and have it open a pane, similar to options pane, that has the fields you can edit. Then that can be expanded on way easier in the future for additional configuration options since its a big pane

I, personally think this one is better because as you pointed out, it's easier for additional configuration in the future and I think, it keeps the UI less cluttered so it makes for good UX.

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

Awesome! I'll start working on that once I can peel my eyes away from this craziness...

image

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

Damn!

Anyways, once you're settled, you might wanna look into;

  • adding kagi into the config:
    maybe the candle entry can become chart | chart_type with the options line | candle | kagi
  • and also the CLI flag:
    --candle can become --chart <line|kagi|candle>

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

  • adding kagi into the config:
    maybe the candle entry can become chart | chart_type with the options line | candle | kagi

  • and also the CLI flag:
    --candle can become --chart <line|kagi|candle>

Great call!

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

Also, I noticed Volumes 'v' gets disabled when kagi is switched to but doesn't get re-enabled (if previously active) when you switch out of it.

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

Also, I noticed Volumes 'v' gets disabled when kagi is switched to but doesn't get re-enabled (if previously active) when you switch out of it.

Nice catch, I just fixed that.

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

--chart-type / -c CLI option and chart_type: _ config option have been added

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

FYI, I am probably migrating to this pattern match for handling keys. There are instances where modifiers show up and I don't care about them (capitalized letters), and with this, I can match on _ for the modifier and only have to specify the KeyCode match once.

    match (key_event.code, key_event.modifiers) {
        (KeyCode::Char('c'), KeyModifiers::CONTROL) => {
            cleanup_terminal();
            std::process::exit(0);
        }
        (_, _) => {}
    }

Edit: A current example is '?'... some terminals have it come across as just KeyCode('?') and no modifiers, others KeyCode('?') and Modifiers::SHIFT. You can see how I have it defined twice currently. This should let me do this just once:

        (KeyCode::Char('?'), _) => {
            app.previous_mode = app.mode;
            app.mode = app::Mode::Help;
            let _ = request_redraw.try_send(());
        }

@miraclx
Copy link
Contributor

miraclx commented Feb 24, 2021

Absolutely! I'm all for that! I wanted to point it out with #51 and #71 because I noticed a lot of code duplication for generally useful bindings but ... didn't.

We could extend this to catching the bindings on multiple panes even, to further reduce the duplication.

With this worked out, I can open another PR to scratch an itch I noted down earlier (ability to access useful toggles while in the Options pane)

@tarkah
Copy link
Owner Author

tarkah commented Feb 24, 2021

Yeah, there are definitely some "global" keybinds that we can match on before dispatching to the handle_events of specific modes. The exception is the AddStock mode since it's text entry based. We can't really have '?' or 'q' as globals there, but we can handle this mode prior to globals prior to all other modes.

And definitely feel free to scratch that itch. The contributions are welcomed and appreciated!

@tarkah
Copy link
Owner Author

tarkah commented Feb 25, 2021

This is taking forever to implement... but pretty much there (formatting is NOT final)! Wait until the end of the gif and you'll see that an error message comes up when submitting invalid values

2C9cZUUDZW

@tarkah
Copy link
Owner Author

tarkah commented Feb 25, 2021

Ok, I think I have the config screen in a good state. @miraclx What do you think?

hiTDKN8Tor

Last thing I need to do is add ability to add these to the config file per symbol (hashmap), and source the form from those values if supplied (this will be very easy to do)

EDIT: I guess I need to add toggle between using close prices vs high / low too. Currently it's just using close prices.

@alfredo-catalano
Copy link

Looks great! Yes, kagi based on high/low.

@alfredo-catalano
Copy link

Cory, nice work...

@tarkah
Copy link
Owner Author

tarkah commented Feb 25, 2021

@alfredo-catalano thanks!

@alfredo-catalano & @miraclx I'm finished! I've added config persistence for these options, as well as the high / low price type.

I've checked the 5Y Kagi against stockcharts.com for both close and high / low and it appears to be spot on. Let me know what you think!

Here is how you can setup the config....

# Ticker options for Kagi charts
#
# A map of each ticker with reversal and/or price fields (both optional). If no
# entry is defined for a symbol, a default of 'close' price and 1% for 1D and 4%
# for non-1D timeframes is used. This can be updated in the GUI by pressing 'e'
#
# reversal.type can be 'amount' or 'pct'
# price can be 'close' or 'high_low'
#
#kagi_options:
#  SPY:
#    reversal:
#      type: amount
#      value: 5.00
#    price: close
#  AMD:
#    price: high_low
#  TSLA:
#    reversal:
#      type: pct
#      value: 0.08

@tarkah
Copy link
Owner Author

tarkah commented Feb 25, 2021

@alfredo-catalano I'm going to spoil you, I've got reversal type implemented so it can be defined per time frame OR ALL time frames:

#kagi_options:
#  SPY:
#    reversal:
#      type: amount
#      value: 5.00
#    price: close
#  AMD:
#    price: high_low
#  TSLA:
#    reversal:
#      type: pct
#      value: 0.08
#  NVDA:
#    reversal:
#      1D:
#        type: pct
#        value: 0.02
#      5Y:
#        type: pct
#        value: 0.10

Now when using the Edit 'e' pane, it'll persist those change for JUST the selected time_frame

@alfredo-catalano
Copy link

I know talent when I see it...

this doesn't work currently, but no reason to redo it when
#95 needs to be merged first
and we can rebase the proper change to that
@tarkah tarkah marked this pull request as ready for review February 26, 2021 00:24
@tarkah
Copy link
Owner Author

tarkah commented Feb 26, 2021

@miraclx I backmerged your keybind changes to this branch. Can you please help double check I didn't mess anything up?

The new keybinds for the ConfigureChart is in the same precedence area as DisplayOptions. I just had to add a check against 'c' since we don't want to toggle off the chart type while configuring it.

miraclx and others added 7 commits February 26, 2021 09:04
* use more intuitive keybindings

up/down or tab/backtab to switch widgets

left/right to choose options

* increase info/error message box to contain all entries
* disable q:quit with kagi config pane open

* reassign q binding to close kagi config pane
* allow scrolling chart while on chart config pane

* allow scrolling chart while on options pane
@tarkah
Copy link
Owner Author

tarkah commented Feb 26, 2021

@miraclx I think we have this pretty well tested now. Anything else or am I good to merge and release?

@miraclx
Copy link
Contributor

miraclx commented Feb 26, 2021

Oh, I'm pretty good on this. +1 from here.
Nice work man!

@alfredo-catalano
Copy link

Awesome work guys!

@tarkah tarkah merged commit 69f76a5 into master Feb 26, 2021
@tarkah tarkah deleted the feat/kagi branch February 26, 2021 17:42
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