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

Feature/1463 insurance #1699

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

Conversation

feyyd
Copy link
Contributor

@feyyd feyyd commented Aug 19, 2018

#1463 I used priceViewFull as example so that this view is doing things as consistently as possible. I'm not sure if pyfa.py is the right place to load insurance data, and would be open to suggestions on where to put it. Another note, the get function in ESI access was modified so that the Insurance could use it, but perhaps it needs its own function for clarity/readability. And one more note, context menu options were added for column display, but it requires restart with the way populating the stats panes work.

@blitzmann
Copy link
Collaborator

o/ Sorry, this dropped off my radar!

So, first thing, thanks for attempting to tackle this! Including insurance prices has definitely been on the radar for a bit, and this seems to work in general. This is a strong first step, but lets make it even stronger!

I'll start with a review current progress, and move into what I think still needs to be done

  1. The implementation of this as a stats view. Please forgive me, I'm about to go on a small rant here >_>. As someone with a background in UI / UX, I constantly ask myself why I'm developing a feature, what information do I need to represent, how to represent it, and what value it brings to me as a user. While this view gets the information across, I personally don't think this is the kind of information that is really necessary to be displayed on the sidebar. It's just too much data for a pretty niche interest. Additionally, while it might be interesting info to display somewhere, do users generally need to know all the insurance tier prices at a glance like this? IIRC (I don't play the game anymore), if you didn't select Plat you were doing it wrong. So to me seems like a lot of data to show that we don't need to show, in an area that is already hurting for space. We have to consider the fact that pyfa is primarily a fitting tool - devoting ~1/6 of the sidebar to the price of insurance seems a bit much 😛

All that being said, I understand that there wasn't any previous discussion on where this data should go, so it's reasonable to think the stats view as it's easy to develop for. And there is the option of turning it off / on (but we also need to consider the fact that if it's turned off, there's currently no way of accessing that information at all - see below for discussion on this). I'm okay with keeping the view as it is, but with the caveat that it should be off by default, and have user's opt in if they have a need to see this information on a regular basis.

  1. The fetching of data. I'm aware there's a bit of a question on where's the best place to actually do it. It's good that you threaded it out - that's an absolute must, so awesome work on that 👍 I feel like the fetching of data should be done in the Insurance services __init__. The service can then be initialized in the MainFrame (for lack of a better spot)(unless you want to forgo an explicit spin up, and rather simply wait for something to call it, in which case it'll spin up when needed via getInstance())

  2. Code-wise, this is an excellent PR. Code is clean and concise, and I don't really see any issue (other than the previously noted discussion on where to launch the thread). Looks like you're implementing a new service which is exactly the right way of doing this. 😎 There's a few things that we can work on together if need be, such as refresh the view when changing which columns show (I don't think this would be difficult)


So that covers the current code progress. All in all, I see good initial set up here. Lets talk about next steps 😄

  1. I know I suggested caching insurance data in the database, however I'm going to step back on idea that a bit. I'm totally fine with this stuff being in memory (and in fact I think I prefer it that way - anything to reduce complexity). However, we do need a way to cache the data so that we don't make a call every time pyfa loads. There are two options I think:
    a) Store this as part of our settings manager. I think, so far, we've only ever saved scalar values into our setting framework, so I'm actually not sure if it would be possible to store a list of objects like what we have here. Something to think about
    b) As an alternative, write a bit of code that saves/loads a json file from the user's save directory that contains the last fetched insurance prices. This could be implemented as part of the Insurance Service loading. I think some did this with the jargon feature - take a look at that as an example.

We can also save the last fetched date, and then come up with a common sense cache time (48 hours? how often does insurance price substantially change?). If the timer has passed when loading the Service, go ahead and load the old data, but then kick off an update as well

  1. There needs to be a way to see the insurance data even when the stats view is off. My thought would be to implement a new tab in the ships Item Stats window - one that is like the attributes tab, and has basically all the columns that the current stats view has.

  2. We should consider adding this to the overall price in the price pane as well. This was the original thought behind including insurance prices - to see how much your ship will actually cost to purchase. However, this is also a bit more complex, as it deals with the pricing bullshit (which we both know if already funky 😛), and it also would involve the whole "included in price" settings, as well as developing out a setting for "default" tier that the user can select from. I think I can help with this one as I have an rough idea on how it would be implemented, and I can use the current code as a base 😄

I think if we can get those additional things implemented and tweaked, we would be in good shape to merge this in for the next release! 🎉

Also, another question: What happens if the insurance service is called upon for prices, but the api fetch hasn't completed yet? If we haven't tested this, then it's something to think about. Also need to consider not being able to access the API for some reason (firewall, no internet, etc) and make sure things don't break.

@feyyd
Copy link
Contributor Author

feyyd commented Sep 24, 2018

Excellent, thanks for taking a look at this. I've moved to a different state so I've haven't had time to look at Pyfa in a while, but I'm back and mostly setup in the new pad. To point 1.1, I agree, showing all prices is super clunky, I think In the application I have I only display platinum, almost everyone I asked either gets plat or no insurance. It does seem like a huge amount of information and they probably only want the plat. I do like the idea of making it opt in, because most people probably won't even want this.

I believe insurance is calculated daily at downtime, basing off that days mineral prices. (Not 100% sure though, can't find a source) I'll probably end up running some tests hitting the ESI endpoint every 10 minutes and just checking when the values change.

I'll look into all the other things you've mentioned here when I work on this next, thanks for the thought out response/analysis.

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