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

Crius mac oldenly patch 1 #3186

Closed
wants to merge 12 commits into from
Closed

Crius mac oldenly patch 1 #3186

wants to merge 12 commits into from

Conversation

CriusMacOldenly
Copy link
Contributor

@CriusMacOldenly CriusMacOldenly commented Oct 21, 2019

Brief overview of PR changes/additions

Fix for Scrollbar does not move with mousewheel/pg scrolling #612

Motivation for adding to Mudlet

I was scrolling with the mouse wheel, and was greatly bothered when I clicked on the scroll bar and it reset the scroll position as if I had never used the mouse wheel to scroll at all.

Other info (issues closed, discussion etc)

This change has my previous changes because this seems to be a patch, with no ability to do my own new fork/pull request, and it was much easier to fix this bug than figure out what github wanted.

@CriusMacOldenly CriusMacOldenly requested review from a team as code owners October 21, 2019 08:13
@add-deployment-links
Copy link

add-deployment-links bot commented Oct 21, 2019

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@vadi2
Copy link
Member

vadi2 commented Oct 21, 2019

Could you please fill in the title and description for this PR?

@Kebap
Copy link
Contributor

Kebap commented Oct 21, 2019

Could you please fill in the title and description for this PR?

There is some information in the original issue here: #612 (comment)

I have issued a fix for this problem in #3186 .
The entire changes for this fix are the differences between the original text selection patch, and patch 1.
If you don't want the #612 fix, don't use patch 1. If you do want the #612 fix, you can merge it in without the text selection fix which is the original patch. I'm certain the change is small enough that you can see the changes between the text selection fix and the scrollbar fix.

@Kebap
Copy link
Contributor

Kebap commented Oct 21, 2019

This PR seems somewhat strange.
You have included all changes from your "development" branch, that is #3166, into this branch here as well. This is not very helpful.
Instead, the changes you intended for #612 should come without the rest of the changes before.
Please let me know if you need additional support here.

Also, your last commit (the only relevant here) seems to add a new file in the root folder, and not modify the existing file in src/ directory.
Please review and adjust accordingly.

Thanks for looking into this!

@vadi2
Copy link
Member

vadi2 commented Oct 21, 2019

We covered this in Discord today - @CriusMacOldenly is super new to Git and they wanted to include changes from the other PR in this one, so we'll go easy :)

@Kebap
Copy link
Contributor

Kebap commented Oct 21, 2019

Did you also discuss the new/updated file issue?

@vadi2
Copy link
Member

vadi2 commented Oct 21, 2019

Yeah... enough pain from git for @CriusMacOldenly for today I think, I'll sort this out 😅

@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Also - your Git (or your code editor) is incorrectly set up as it is putting in carriage-returns as well as line-feeds (Window style End-Of-Lines) at the end of each line instead of the standard which we used which is just line-feeds (*nix style End-Of-Lines). Which ballses things up for doing diffs on the files to extract the changes...

@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

I am just putting together a PR against your proposal that should clean things up here... wait a few minutes please...

@CriusMacOldenly
Copy link
Contributor Author

I made certain a checkmark was placed in:
Allow edits from maintainers.
It shouldn't be too bad really. I have no plans to do any other coding on mudlet at this time, so any changes you make will survive the test of time.

@SlySven SlySven added the 0 - Waiting for other PR Merge This PR is awaiting further commit(s) from a PR into the Contributor's own repository. label Oct 25, 2019
@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Okay can you go to https://github.com/CriusMacOldenly/Mudlet/pull/1 ... and follow the instruction I have offered...

Convert: change Windows EOL to Unix EOL in previously modified file
@SlySven SlySven added 1 - Ready for review and removed 0 - Waiting for other PR Merge This PR is awaiting further commit(s) from a PR into the Contributor's own repository. labels Oct 25, 2019
@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Success! Now the "Files changed" tab at the top of the page shows the actual changes you want to make...

@CriusMacOldenly
Copy link
Contributor Author

CriusMacOldenly commented Oct 25, 2019

Indeed. I can now see 6 lines I commented out, but forgot to delete. :( Should have only been +92 -198 in the end. I'd edit the file again and remove them, but that'll probably bother you a lot.

Copy link
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

I've taken a quick look at this - and thought "this is not going to be a simple one to review" - I am sorry but apart from several good cups of coffee I think it is going to take a while for me to get back into gear over the code in this bit and I fear I will not be able to give a meaningful reply without some serious playing with it over the next few days. The good news is that I do have some time off for all of the next fortnight...

src/TTextEdit.cpp Show resolved Hide resolved
@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Before pushing another change out to your github repository I wonder if there is a way to see whether there are CR-LF or just LF on the ends of the lines in the files that get changed. I note that it was only the TTextEdit.cpp file that got messed up, even though you had edited both it and the TTextEdit.h file... 🤔

@CriusMacOldenly
Copy link
Contributor Author

You need to separate declaring y1 and assigning it because normalizeSelection() assigns mPA, which is then assigned to y1.
The header also had the incorrect line ends but Vadi modified both the h and cpp file's line endings, then I only altered the cpp afterwards... which left the h file still with the Vadi altered line endings.

@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Humm, 🤔 so every file you touch gets converted to Windows style end-of-lines and committed as such.

If you go to the place in your file-system where the Mudlet source code is and type in a command line:

git config core.autocrlf

does it report anything?

@CriusMacOldenly
Copy link
Contributor Author

D:\Mudlet>git config core.autocrlf
true

@CriusMacOldenly
Copy link
Contributor Author

Any other changes before I commit your requested y1 declare and assign change?

@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Please hold... you are in a queue and the next available operator will be with you as soon as possible... 🙂

@CriusMacOldenly
Copy link
Contributor Author

Oh, I already committed the change you requested, and deleted the commented out lines like I wanted.
If there are more changes wanted, I'll just make them, and upload them again. It's all good.

@SlySven
Copy link
Member

SlySven commented Oct 25, 2019

Your call is important to us, please hold and the next available operator will be with you as soon as they can - alternatively you might like to check out our on-line presence on the Interweb thingy...

I'm discussing the EOL issue on Discord - if you want to go to the #help channel mayhaps we can nail this problem there...

@CriusMacOldenly
Copy link
Contributor Author

This patch introduces an issue of a quick flicker where the split pane appears and disappears when starting to scroll up with the mouse alone. Vadi linked a video in https://streamable.com/jycjg .

@CriusMacOldenly CriusMacOldenly deleted the CriusMacOldenly-patch-1 branch October 26, 2019 19:14
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

4 participants