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

[NETSHELL] Remove erroneous DNS Server line and fix date/time format #2655

Merged
merged 1 commit into from Apr 28, 2020
Merged

[NETSHELL] Remove erroneous DNS Server line and fix date/time format #2655

merged 1 commit into from Apr 28, 2020

Conversation

ghost
Copy link

@ghost ghost commented Apr 24, 2020

Purpose

_ Current Network information does not rely on system / user settings for date and time formatting
_ Current Network information always insert a "blank" DNS server line before the actual one.

JIRA issue: CORE-16947

image

Proposed changes

_ Use correct formatting
_ Remove erroneous DNS line

image

@ghost ghost requested a review from learn-more as a code owner April 24, 2020 14:04
Copy link
Member

@binarymaster binarymaster left a comment

Choose a reason for hiding this comment

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

There are two code chunks that are repeated. Please make a function for it.

dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
@binarymaster binarymaster changed the title [SHELLEXT] Fix of incorrect DNS Server reporting and wrong date/time formatting (CORE-16947) [NETSHELL] Fix incorrect DNS Server reporting and wrong date/time format Apr 24, 2020
@binarymaster binarymaster added the bugfix For bugfix PRs. label Apr 24, 2020
@ghost ghost requested a review from binarymaster April 24, 2020 14:37
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.h Outdated Show resolved Hide resolved
@ghost ghost requested a review from binarymaster April 24, 2020 15:09
@ghost
Copy link
Author

ghost commented Apr 24, 2020

@binarymaster : Thanks !

I was not able to reproduce locally the last comment. Committed on Git on your proposal.
All comments OK now. OK for merge ?

Copy link
Member

@binarymaster binarymaster left a comment

Choose a reason for hiding this comment

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

LGTM. Let's wait for some more reviews from others.

@binarymaster binarymaster changed the title [NETSHELL] Fix incorrect DNS Server reporting and wrong date/time format [NETSHELL] Remove erroneous DNS Server line and fix date/time format Apr 24, 2020
@ghost ghost requested a review from yagoulas April 24, 2020 17:22
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
@ghost ghost requested a review from sdever April 25, 2020 09:16
@binarymaster
Copy link
Member

@KyleKatarn31 I fixed whitespace problems and squashed all changes into one commit. If you will need to update your code again, please do hard reset to my commit and continue your work from it.

I already did this yesterday, but you applied existing commits on top of it, so my fixes got broken again.

@ghost
Copy link
Author

ghost commented Apr 25, 2020

@KyleKatarn31 I fixed whitespace problems and squashed all changes into one commit. If you will need to update your code again, please do hard reset to my commit and continue your work from it.

I already did this yesterday, but you applied existing commits on top of it, so my fixes got broken again.

OK thanks ! I hope I won't have any additional change to apply :-)

dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
@ghost ghost requested a review from sdever April 26, 2020 08:29
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
@ghost ghost requested a review from ThFabba April 26, 2020 09:08
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
@ghost ghost requested a review from sdever April 26, 2020 09:13
@HBelusca HBelusca self-assigned this Apr 26, 2020
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
dll/shellext/netshell/lanstatusui.cpp Outdated Show resolved Hide resolved
@binarymaster
Copy link
Member

OK thanks ! I hope I won't have any additional change to apply :-)

@KyleKatarn31 you ignored what I said earlier: #2655 (comment)

You rewrote my changes that already had _countof fixes and more: 522e140

@HBelusca HBelusca removed their assignment Apr 26, 2020
@ghost
Copy link
Author

ghost commented Apr 26, 2020

OK thanks ! I hope I won't have any additional change to apply :-)

@KyleKatarn31 you ignored what I said earlier: #2655 (comment)

You rewrote my changes that already had _countof fixes and more: 522e140

I thought that I could finally use the explicit length since it's now basec on dynalloc. I'll rework again following the feedback from all of you. Thanks !

@ghost ghost requested a review from HBelusca April 26, 2020 12:45
@ghost
Copy link
Author

ghost commented Apr 26, 2020

Fixed, tested, fully operative.
@HBelusca : is the implementation based CString as you requested ?
@learn-more : as code owner, do you approve current solution ?

@binarymaster binarymaster merged commit 02d856c into reactos:master Apr 28, 2020
@ghost
Copy link
Author

ghost commented Apr 29, 2020

Thanks !

@ghost ghost deleted the CORE-16947 branch April 29, 2020 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix For bugfix PRs.
Projects
None yet
8 participants