-
-
Notifications
You must be signed in to change notification settings - Fork 386
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
Update the flightlog rebased #5666
Update the flightlog rebased #5666
Conversation
So rebase, or we can just squash merge, or do we want to keep all commits? E.g I see new functions like |
@Web-eWorks , I think this is ready for final review, although looks like it'll need rebasing again on master. |
cb8e289
to
920abf1
Compare
Rebased. I'm wondering if I'll hit some clang format errors now ? |
Yeah, hit some clang format errors, I'll have to sort them out later in the week - sorry |
@Web-eWorks , this should now be ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite happy with the effect and direction of this PR. I do apologize for not getting through the review until now - my schedule has been quite busy and I've accidentally let things slip through the cracks like this review.
I've left (several!) comments where there are things I'd like to see addressed, either as part of this PR directly or in a follow-up PR if you're not confident in being able to address them all in the next ~2 weeks. We'll be entering formal "feature freeze" on the 27th and anything not bugfix/stability related will be put on hold at that point until the release.
Overall I consider this PR to be functional enough to merge as part of the release, but in addition to the nitpicks that comprise most of the comments I've left there are a few maintainability concerns regarding serialization and dataflow that I'd like to see addressed long-term.
001607a
to
e8b050a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for addressing the last round of review comments. I've gone over the diff again and only one additional area has stood out to me as something that could be improved.
Before merge, I'd like you to address the table[key, value] -> table<key, value>
annotation issues (they're not high severity but they are very quick to fix) and you indicated prior you intended to address the NaturalDocs formatting - otherwise, the remaining outstanding issues are an "at your discretion" priority.
I plan to merge this PR sometime Sunday evening/night EST - hopefully that gives you enough time to address the remaining issues. If not, let me know any time and I'll merge sometime next week.
e56c355
to
29d4757
Compare
Looks good to me, thanks for addressing that formatting issue. At this point I'd recommend rebasing and squashing together related changes / iterative improvement commits to clean up the history (in the name of appeasing @impaktor, the resident git fascist 😄). |
@Web-eWorks following up on your recommendation to tidy up counting elements and I found a bug and another issue I'm tidying. Hope to have it done in the next 3-4 hours |
@JonBooth78 bump - ETA on final squash + rebase? I'd like to merge this before #5706. |
7c44b0b
to
2fb6607
Compare
f0c97c4
to
dfda65d
Compare
dfda65d
to
0f20d80
Compare
0f20d80
to
920866d
Compare
… it was removed from
de55e68
to
de57ab9
Compare
This also moves FlightLog.lua to modules/FlightLog/FlightLog and splits out new functionality, moving some logic from the UI code into the base functionality This changes the way that flightlogs are persisted but will still load the old version TODO: word-wrapping multi-line text editing of custom text entries
de57ab9
to
f5e84b2
Compare
@Web-eWorks , this is all tidied up ready to merge. Apologies it took so long, I was trying to do it step by step and kept tripping over the file split that git failed to track (well github refused to track for pushing new changes to the PR, hence having to create this second one in the first place) - oh that and a bug in my git client (which I've updated and now it's gone away) . In the end realized that if we pulled all the non-flightlog related changes ahead of all the flightlog related ones and the rename that we can track, then the rest can be a single, stand alone commit. |
@JonBooth78 started a new game from the latest commit, initial entry is missing its custom description. |
Fixed - it was an errant colon in the function declaration for (My editor apparently cleaned up a lot of trailing whitespace on save too - if using VSCode I'd definitely recommend turning on the Trim Trailing Whitespace and Insert Final Newline options...) |
Thanks!
…On Fri, 26 Jan 2024, 21:38 Webster Sheets, ***@***.***> wrote:
Fixed - it was an errant colon in the function declaration for
FlightLog.MakeCustomEntry 😄
—
Reply to this email directly, view it on GitHub
<#5666 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB44LZOXEXDAEUHGVH4MNDYQNTSFAVCNFSM6AAAAAA7RNIQC2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGY3TGOBRHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
User facing changes:
_Note: This replaces / closes #5636 as due to the way I did some merging along the way (since rebased away and hidden in this branch), I could no-longer update that PR.
Note: Screenshots below have alpha baked into them so the background looks a little off on the export window as that grey is actually transparent so the background actually appears black
Export Collapsed
Export Expanded
Description
Potentially controversial changes I’m aware of:
imgui
doesn't support word-wrapping entry boxes this would feel nicer and mitigate that a bit too.Things I’ve tested:
Things I am considering to doing later:
system
andstation
events are container events that can contain events that happened when within that system or station. System events of course will contain station events.