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

Support for recorded and paused time #225

Merged
merged 230 commits into from
Sep 13, 2020
Merged

Conversation

FJBDev
Copy link
Collaborator

@FJBDev FJBDev commented Sep 10, 2020

Changes per code review suggestions in previous PR

@wolfgang-ch
Copy link
Collaborator

@FJBDev Let me know when I should do a review.

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 12, 2020

@FJBDev Let me know when I should do a review.

You can do a review :-)

@wolfgang-ch wolfgang-ch merged commit d0487c5 into mytourbook:FJBDev Sep 13, 2020
@wolfgang-ch
Copy link
Collaborator

The code formatting do not join lines, so there can be "line breaks".

In the beginning the join line feature was tuned on but there was an issue (which I can't remember) that I turned it off. Maybe in later Eclipse versions this can be setup more granularly as I also hate to join the lines manually.

This is not an issue to not merge it into main branch

mt-join-lines

mt-never-join-lines

@wolfgang-ch
Copy link
Collaborator

Since a while I'm using the state contant name also for the state key, this is easier to use by just copy/paste it :-)

mt-pref-values

@wolfgang-ch
Copy link
Collaborator

When using UI.EMPTY_STRING then it is better to use it from net.tourbook.common.UI because when using other net.tourbook.common.UI resources then the fully qualified name for UI will be used

mt-ui-common

@wolfgang-ch
Copy link
Collaborator

tourRecordingTime is also renamed in the table TourCompared, I think this was not intended as the database update method do not reflect this renaming. I've not tested it but I think, MT will fail when it is accessing this table.

mt-tour-compared-2

mt-tour-compared

Copy link
Collaborator

@wolfgang-ch wolfgang-ch left a comment

Choose a reason for hiding this comment

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

After 2.5h of code review, I need to rest my brain as I'm getting tired. This is the reason why I can't work 100% after my bicycle accident. I think what makes me tired are the many "context" switches during the review as there are so many different files to look at.

Here you can see my current review location, I started from top to bottom and collapse all which is reviewed

mt-review

@wolfgang-ch wolfgang-ch mentioned this pull request Sep 13, 2020
@wolfgang-ch
Copy link
Collaborator

tourRecordingTime is also renamed in the table TourCompared, I think this was not intended as the database update method do not reflect this renaming. I've not tested it but I think, MT will fail when it is accessing this table.

mt-tour-compared-2

This is the corresponding error when expanding a year in the ref tour view

mt-sql-exception

Copy link
Collaborator

@wolfgang-ch wolfgang-ch left a comment

Choose a reason for hiding this comment

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

The code review is now done with 1 major issue, I'll do a functional review when the code is OK

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

The code review is now done with 1 major issue, I'll do a functional review when the code is OK

OK, i will fix the major issue and the other code suggestions quickly.

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

Since a while I'm using the state contant name also for the state key, this is easier to use by just copy/paste it :-)

mt-pref-values

Fixed

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

The code formatting do not join lines, so there can be "line breaks".

In the beginning the join line feature was tuned on but there was an issue (which I can't remember) that I turned it off. Maybe in later Eclipse versions this can be setup more granularly as I also hate to join the lines manually.

This is not an issue to not merge it into main branch

mt-join-lines

mt-never-join-lines

Fixed

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

When using UI.EMPTY_STRING then it is better to use it from net.tourbook.common.UI because when using other net.tourbook.common.UI resources then the fully qualified name for UI will be used

mt-ui-common

Fixed

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

tourRecordingTime is also renamed in the table TourCompared, I think this was not intended as the database update method do not reflect this renaming. I've not tested it but I think, MT will fail when it is accessing this table.
mt-tour-compared-2

This is the corresponding error when expanding a year in the ref tour view

mt-sql-exception

I have

The code review is now done with 1 major issue, I'll do a functional review when the code is OK

Ok, I have a fixed this 1 major issue here.

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

tourRecordingTime is also renamed in the table TourCompared, I think this was not intended as the database update method do not reflect this renaming. I've not tested it but I think, MT will fail when it is accessing this table.

mt-tour-compared-2

mt-tour-compared

I have renamed the field in the database. I had forgotten to do so.

@FJBDev
Copy link
Collaborator Author

FJBDev commented Sep 14, 2020

After 2.5h of code review, I need to rest my brain as I'm getting tired. This is the reason why I can't work 100% after my bicycle accident. I think what makes me tired are the many "context" switches during the review as there are so many different files to look at.

Here you can see my current review location, I started from top to bottom and collapse all which is reviewed

mt-review

Indeed, lots of changes and work for this feature. I started several weeks ago now :-)

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.

None yet

2 participants