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

[MAGNIFY] Fix the magnification ratio bug for CORE-14946 #823

Merged
merged 3 commits into from Aug 27, 2018

Conversation

LuRenJiasWorld
Copy link
Contributor

This bug was caused by leaving the magnify window unfreshed when trigger the "Magnification Level" ComboBox.

Fixed it.

JIRA issue: CORE-14946

(Sorry for repost this Pull Request)

Author: Benjamin Chris
Email: loli@lurenjia.in
(It's my real name and real email, I promise.)

@HBelusca
Copy link
Contributor

(It's my real name and real email, I promise.)

This should already be visible in the patch :) (click on the commit, append .patch to the URL and you can see what's reported in the patch).

@@ -350,7 +351,7 @@ void GetBestOverlapWithMonitors(LPRECT rect)

if (rcTop > (rcMon.bottom - rcHeight))
rcTop = (rcMon.bottom - rcHeight);

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra trailing whitespace, please remove.

@@ -923,9 +924,15 @@ INT_PTR CALLBACK OptionsProc(HWND hDlg, UINT message, WPARAM wParam, LPARAM lPar
if (HIWORD(wParam) == CBN_SELCHANGE)
{
HWND hCombo = GetDlgItem(hDlg,IDC_ZOOM);
LPCTSTR currentZoomValue = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the indentation by remaining consistent with the rest of the file: no tabs, 4-spaces.

LPCTSTR currentZoomValue = "";

/* Get index of current selection and the text of that selection */
unsigned int currentSelectionIndex = ComboBox_GetCurSel(hCombo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just use int.

/* Get index of current selection and the text of that selection */
unsigned int currentSelectionIndex = ComboBox_GetCurSel(hCombo);
ComboBox_GetLBText(hCombo, currentSelectionIndex, currentZoomValue);
iZoom = atoi(currentZoomValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace atoi by _ttoi.

@LuRenJiasWorld
Copy link
Contributor Author

Sorry for my negligence. I'll fix that.

@HBelusca
Copy link
Contributor

Hi, you haven't resolved the "conflict", as you still have the conflicting lines in your commit(s).

@LuRenJiasWorld
Copy link
Contributor Author

Hi, you haven't resolved the "conflict", as you still have the conflicting lines in your commit(s).

I think now it should be solved now.
Actually I've never been involved in the development of open source projects like this before, so I'm like... totally lack of experience.
Sorry for that.

@@ -18,6 +18,8 @@
#include <winnls.h>
#include <shellapi.h>
#include <windowsx.h>
#include <stdlib.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is stdlib.h really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's needed, because when i try to delete this line, MSVC compiler gives me an error message "atoi undefined".
It seems that the _ttoi function is a marco defined in tchar.h, which will convert _ttoi automatically into atoi or wtoi depends on the environment.

@@ -923,9 +925,15 @@ INT_PTR CALLBACK OptionsProc(HWND hDlg, UINT message, WPARAM wParam, LPARAM lPar
if (HIWORD(wParam) == CBN_SELCHANGE)
{
HWND hCombo = GetDlgItem(hDlg,IDC_ZOOM);
LPCTSTR currentZoomValue = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

TEXT("").

@LuRenJiasWorld
Copy link
Contributor Author

Yes, you are right. Thanks for your correction, you are so patient.

And I think I should be more carefully, i.e, double check my code, especially the format before I submit a Pull Request.

Anyway, you're really doing something absolutely great! I think I'll try my best to continually contribute to it in the future.
👍

@HBelusca
Copy link
Contributor

Yes, you are right. Thanks for your correction, you are so patient.

You're welcome; and so that for your future PRs you will know what to look for 😃

It seems that the _ttoi function is a marco defined in tchar.h, which will convert _ttoi automatically into atoi or wtoi depends on the environment.

Heh you are correct, I've just noticed that we compile the magnifier as an ANSI app (instead of UNICODE).

@HBelusca HBelusca merged commit 85bbd69 into reactos:master Aug 27, 2018
@HBelusca
Copy link
Contributor

Ah, I've just noticed that when committing your PR, the resulting commit doesn't have your human-readable author name+email. This is due to the problem of Github using Github's registered name as the author name instead of the original patch's author name, when doing a Squash+Commit operation.
The only known fix is to update your Github's registered name.

@gigaherz
Copy link
Member

Actually the REAL fix is for PR authors to squash and rebase their code themselves, so that we don't have to use the "squash merge" option. Having your name+email as the default in github is only a workaround.

@LuRenJiasWorld
Copy link
Contributor Author

Yeah, I think I will take both of your advice, and do better next time. : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants