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

SYMBIAN: Minor fixes for comments and code #3170

Closed
wants to merge 19 commits into from

Conversation

@digitall
Copy link
Member

@digitall digitall commented Jul 19, 2021

This is a fixed up version of #3167 with a cleaned up history, squashed commits together for same file and split the comment cleanup for the SDL, LOG and KEYMAPPER to separate commits. This should be much easier and clearer to review.

huhungahuru added 19 commits Jul 17, 2021
Correcting the code format according to the guidelines.
- Line 48 (const HardwareInput& x, const HardwareInput& y) is replaced by (const HardwareInput &x, const HardwareInput &y).
- Line 57 (const HardwareInput& x) is replaced by (const HardwareInput &x).

Example of another case: #3157
c\resource\help was changed to !\resource\help, to allow installation in any memory of the phone.

- This line of code is valid or not! 
bld freeze lib cleanlib final resource savespace releasables :
Clean program files that are in any memory of the phone.
clear_uninstall = """
- Changed (c:\data\scummvm\scummvm.x) by (!:\data\scummvm\scummvm.x)
@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

@huhungahuru : If you check, the final result of this branch should be even with your original code in terms of changes.

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

@fedor4ever : As you are the Symbian porter, any comment on these changes? OK with you?

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

Most of these changes seem to be just adding full stops to the end of various comments. I don't know if this is desired or required, since https://wiki.scummvm.org/index.php?title=Code_Formatting_Conventions#Use_common_sense ... While some of this formatting is reasonable, generating a large amount of formatting churn can make tracing code changes harder later so not sure about this. @sev- : Any thoughts?

Also, I don't think our commit message guidelines use the full stop at the end. While not critical, I can filter this out to the standard form using rebase before committing this branch: https://wiki.scummvm.org/index.php?title=Commit_Guidelines#Commit_message_formatting

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

Particularly, I think the "fixes" to the vsnprintf header in c247138 should be removed as this header is imported code and cleaning it up from the upstream original unless critically required makes it much harder to track any changes made to it...

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Jul 19, 2021

No ok.

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Jul 19, 2021

You mixed all backends code.
Edit .pl scripts are waste of time. They are gone as port can selfbuld and upload to sourceforge.

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Jul 19, 2021

in backends/platform/symbian/help/build_help.mk should be- copy ScummVM.hlp $(EPOCROOT)epoc32\$(PLATFORM)\c\resource\help. It point to SDK directory.

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Jul 19, 2021

They are trash: backends/platform/symbian/res/scummvm_A0000658.rss and backends/platform/symbian/res/scummvm.rss. Remove freely.

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

@fedor4ever : I assume that "No ok" means that you are not happy with these changes as-is. I am sorry, but it is quite hard to understand you at times. I don't understand what you mean by "You mixed all backends code.". This code has been submitted by @huhungahuru so my only contribution was tidying up the commits into a set of logical changesets rather than a big bucket of commits.

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

@fedor4ever : If you have specific comments, it is best to put those in line comments on the specific commits as it otherwise requires quite a bit of hunting for the exact code you are referring to:
https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-line-comments-to-a-pull-request

@huhungahuru
Copy link

@huhungahuru huhungahuru commented Jul 19, 2021

I think the best option is to cancel this request for changes. I apologize for the problems caused.

@sev-
Copy link
Member

@sev- sev- commented Jul 19, 2021

@fedor4ever Please, be constructive with your comment or do not comment at all!

@huhungahuru thank you for your contribution. However, I would recommend splitting this into several PRs and squash similar ones. E.g. there is a number of commits that are adding full stops and fixing grammar, those could be committed as a single commit.

Any functional changes must go separately.

Apologies, that it creates a bit more work, but we need to be careful and it is not easy to review such big PRs.

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Jul 19, 2021

backends/platform/symbian/src/SymbianMain.cpp: has no sense - // This line was added, we still need to test (07/17/2021).
Revert back setbuf changes.

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

@huhungahuru : No problem. I would suggest doing a quick git course and learning about feature branches since they make it much easier to avoid merge commit issues on your master / main branch:
https://gist.github.com/vlandham/3b2b79c40bc7353ae95a

To echo @sev- comment, it is just hard to review when it is not broken into logical changesets. Also formatting fixes are usually best if they can be produced from an automated tool so it can be shown to be repeatable i.e. http://astyle.sourceforge.net/

@digitall
Copy link
Member Author

@digitall digitall commented Jul 19, 2021

@fedor4ever : Since you have quite a few objections to the changes in this PR and you are the Symbian porter anyway, I am going to close this PR and let you cherry-pick (@huhungahuru : another good git tool https://git-scm.com/docs/git-cherry-pick ) any changes which you think are useful or relevant.

@digitall digitall closed this Jul 19, 2021
@huhungahuru
Copy link

@huhungahuru huhungahuru commented Jul 19, 2021

@fedor4ever : I will make suggestions for changes in my own repository and if the changes convince you, you can add them. Hope I can help with the Symbian port.

@fedor4ever
Copy link
Contributor

@fedor4ever fedor4ever commented Jul 19, 2021

@huhungahuru, great job. But this task should be splitted on several pull request. Several parts I can accepted as is.
@sev- , I try my best to be constructive.
@digitall , thanks for docs.

fedor4ever added a commit to fedor4ever/scummvm that referenced this pull request Jul 20, 2021
Add cleanup from results previous build before new start.
Code style fixes from scummvm#3170.
Forced CPU usage.
fedor4ever added a commit to fedor4ever/scummvm that referenced this pull request Jul 21, 2021
fedor4ever added a commit to fedor4ever/scummvm that referenced this pull request Jul 21, 2021
fedor4ever added a commit to fedor4ever/scummvm that referenced this pull request Jul 21, 2021
bluegr pushed a commit that referenced this pull request Jul 21, 2021
fedor4ever added a commit to fedor4ever/scummvm that referenced this pull request Jul 22, 2021
Install fails if file already exist. They comes to another folder now.
Apply style fix from scummvm#3170
sev- added a commit that referenced this pull request Jul 27, 2021
Add cleanup from results previous build before new start.
Code style fixes from #3170.
Forced CPU usage.
sev- added a commit that referenced this pull request Jul 27, 2021
sev- added a commit that referenced this pull request Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants