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

Rewritten text selection #3166

Merged
merged 18 commits into from
Nov 4, 2019
Merged

Rewritten text selection #3166

merged 18 commits into from
Nov 4, 2019

Conversation

CriusMacOldenly
Copy link
Contributor

@CriusMacOldenly CriusMacOldenly commented Oct 16, 2019

Brief overview of PR changes/additions

commented out the old code regarding text selection, put in my own code for text selection.

Motivation for adding to Mudlet

existing text selection annoyed me as I was trying to copy and paste from mudlet into discord/notepad

Other info (issues closed, discussion etc)

Most of the old code is now commented out. I didn't know if I should remove it completely or not.
The logic of text selection has changed extensively.
I left in place existing code I thought was really poorly written, so it looks like fewer changes.
If you use ctrl to highlight some text, mudlet only deletes the highlight from the sections that would have been selected if you weren't pressing ctrl.

Fix #3064

@CriusMacOldenly CriusMacOldenly requested review from a team as code owners October 16, 2019 00:47
@add-deployment-links
Copy link

add-deployment-links bot commented Oct 16, 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.

@Kebap Kebap mentioned this pull request Oct 21, 2019
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.

It is virtually impossible to review this PR on-line here on GitHub unfortunately as it is very difficult to pick out the actual changes. It will need some git jiggery-pokery to revise the PR so that the noise (possibly from EOL-conversions?) can be filtered out - I don't have the time this week so if someone else wants to jump in before I can get to it, please do so...

@CriusMacOldenly
Copy link
Contributor Author

We know it is the /n/r /r/n /n /r differences between dos and unix.
Unfortunately, no matter how hard I try to alter the eol changes, I will fail, even if I succeed.
My recommendation, of course, is we all adopt an eol that every operating system now, and into the future will use.
Failing that, we need someone to recode git to automatically replace all eol changes for all uploaded files to the operating system version the project is configured to use. (This is the best solution in my opinion).
Failing that, we need to at alter git diff to not care about eol differences. (This is a bad idea)
Honestly though, this problem is only happening because no one has bothered to make this simple bug fix. I mean, for I don't know how long, ftp clients can be configured to alter all eol characters if you upload files as text...

@CriusMacOldenly
Copy link
Contributor Author

Since both the last text selection diff, and the patch 1 diff are both on dos style files, you can view all the differences associated with the scroll bar problem (issue #612) easily. I pasted an image into discord as well - it's a total of 4 line additions.

@vadi2
Copy link
Member

vadi2 commented Oct 24, 2019

Nobody needs to recode Git; it already has a solution to this: https://help.github.com/en/github/using-git/configuring-git-to-handle-line-endings

You just need to use a desktop client to make use of it :)

That said, it's also not helping anything when everyone jumps straight on commenting about the like endings or how messed up the PR is... it does say "First time contributor", please respect that.

@CriusMacOldenly CriusMacOldenly changed the title Improved text selection. Unhighlighting ctrl selects still broken. Improved text selection. Nothing is broken as far as I know. Oct 25, 2019
@vadi2
Copy link
Member

vadi2 commented Oct 26, 2019

@CriusMacOldenly Is the split screen broken for you also? See in https://streamable.com/jycjg how it shows / goes away / shows when I scroll up. It should only show once you scroll up and not hide/show as you're scrolling up more.

@vadi2 vadi2 assigned CriusMacOldenly and unassigned vadi2 Oct 26, 2019
@CriusMacOldenly
Copy link
Contributor Author

I have reviewed your screenshot video, and this problem is reported as "Issues with the scrolling handler #3054". My fix does not fix Issue#3054, and does not attempt to fix Issue#3054. As such, I would expect your reported problem to exist with my code as well. Testing my code does not show any unexpected behaviour, such as fixing Issue#3054.

@vadi2
Copy link
Member

vadi2 commented Oct 26, 2019

Notice how in #3054 the scrollbar never dissapears. That is the new behaviour.

@demonnic can you confirm?

@CriusMacOldenly
Copy link
Contributor Author

CriusMacOldenly commented Oct 26, 2019

I keep looking at your video, and I am never seeing the scrollbar disappearing. :( I have watched your video to the end about 10-15 times during my tests, and I can't see anything unexpected or incorrect in your video. I need a better description of the problem you are seeing.
Please show me a video of the current production image, and a video of the problem with a clear description of the difference between them.
My best guess is you are looking at the patch 1 version where I fixed the scrollbar movement when you didn't click on the scrollbar (which is not this issue nor fix - it is the fix for #612 ), but again, I can't see any problem in your video that is different from Issue #3054 .

@CriusMacOldenly
Copy link
Contributor Author

Ok, I can see your complaint and the difference between my version and the production version of mudlet. That flicker you are seeing is caused by my #3186 fix for #612 . It is completely unattached to this text selection issue #3166 .

…es text selection, and scroll bar changes are in a separate branch.
@vadi2 vadi2 changed the title Improved text selection. Nothing is broken as far as I know. Rewritten text selection Oct 29, 2019
@vadi2
Copy link
Member

vadi2 commented Oct 30, 2019

Please see existing Mudlet - yes, people like it that way

…u needed to move the mouse to select the line(s).
@CriusMacOldenly
Copy link
Contributor Author

I have implemented all changes per your request.
Thank you for taking the time to test and suggest these changes for the improvement of mudlet.

@vadi2
Copy link
Member

vadi2 commented Oct 30, 2019

Thanks for taking your time to do these improvements! :)

@vadi2
Copy link
Member

vadi2 commented Oct 30, 2019

I can confirm both problems I mentioned are gone, will test more 🔨

@CriusMacOldenly
Copy link
Contributor Author

CriusMacOldenly commented Oct 30, 2019

Excellent, thank you for the confirmation. Will wait more Clipboard 1

@CriusMacOldenly
Copy link
Contributor Author

Thank you, that is kind of you.

(use the clang-format tool with the .clang-format file at the root of the repository for this)
@vadi2
Copy link
Member

vadi2 commented Nov 1, 2019

Can you check if ctrl+dragging still selects lines?

@vadi2
Copy link
Member

vadi2 commented Nov 1, 2019

I've fixed the CodeFactor issues, they were mostly just style ones.

@CriusMacOldenly
Copy link
Contributor Author

I was told to ignore the CodeFactor issues. It seems I was given incorrect instructions. :(

@CriusMacOldenly
Copy link
Contributor Author

ctrl+dragging does not select lines. This is strange since I know it used to... before I made the ctrl click selecting a line without movement. :(

@vadi2
Copy link
Member

vadi2 commented Nov 1, 2019

I don't know who gave them, but I don't think it was anyone on the core team. That said we are experimenting with it, it's not always right, and we're tweaking settings - in this case it was right :)

Yes, I think the ctrl+click (which we do need) broke it

@CriusMacOldenly
Copy link
Contributor Author

I commented out the newly added ctrl-click code, and ctrl drag selection works perfectly. So, the newly modified code broke ctrl drag.

@CriusMacOldenly
Copy link
Contributor Author

Crius MacOldenly10/15/2019
I tried to make my code look as similar to other people's, however, github is calling them issues... which I agree with -- but it does match the existing code better... which had all these issues already. XD

s/Mud/Game10/15/2019
Don't mind codefactor too much

This is the advice I went by.

@CriusMacOldenly
Copy link
Contributor Author

After staring at the code a long time, I was finally able to sledge hammer the code into shape.
It now works for both ctrl-click selection and ctrl-drag selection.
Of course, I tested the regular selection too.
I hope you don't find more problems because all the fixing of line feeds is starting to take a significant amount of time. If only Crius would download github desktop. :(

@vadi2
Copy link
Member

vadi2 commented Nov 1, 2019

Can confirm that works.

Wider testing is blocked by a regression, but I fixed that in #3212, so as soon as we merge that in (probably today) more people can test this again. It's looking good!

@Eraene
Copy link
Contributor

Eraene commented Nov 4, 2019

For some reason if you click inside the main window (where the text is) while said window is out of focus, it will automatically highlight the nearest character to wherever the mouse is. There is no mouse drag happening, it just highlights a character upon click when the window is brought into focus.

@CriusMacOldenly
Copy link
Contributor Author

CriusMacOldenly commented Nov 4, 2019

For some reason if you click inside the main window (where the text is) while said window is out of focus, it will automatically highlight the nearest character to wherever the mouse is. There is no mouse drag happening, it just highlights a character upon click when the window is brought into focus.

This is happening with every out of focus click in my text selection version.
This is happening in the production version click about every second click. It seems in the production version 4.2.1, the first out of focus click selects the text, and the second out of focus click removes all selection... then it repeats.
I don't know which is better. :(
This is very different behaviour to if mudlet remains in focus the entire time -- usually (but not always) only an out of focus click seems to do this.
This really worries me because my changes have nothing to do with if mudlet is in focus or not... it simply accepts the events and deals with them.
Basically, it's as if slightly different events are being sent if mudlet is in focus or not.

Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Alright, something we can look at in another PR, maybe something you can help with @CriusMacOldenly?

This one is good to go, thanks so much for working on it!

@vadi2 vadi2 merged commit 1436cfc into Mudlet:development Nov 4, 2019
@CriusMacOldenly
Copy link
Contributor Author

It'll have to be a different PR since it looks like you merged this PR now.
I won't bother spending the time to look at this since it'll be a different PR.

@Kebap
Copy link
Contributor

Kebap commented Nov 5, 2019

Yes this PR is closed. You can delete your branch. Any new PR would again stem from development then.
I will copy @Eraene's observation into a seperate issue, so it does not get lost here, and can be fixed later.

edit: No, you can't delete your branch, because you never made one before starting this PR. Instead you worked on your development branch. That seems a bit unusual, because it makes it harder to be working on different PRs simultaneously.

dicene pushed a commit to dicene/Mudlet that referenced this pull request Feb 19, 2020
* Improved text selection. Unhighlighting ctrl selects still broken.

* Removed the commented code.

* Removed the commented out code.

* Uploaded to wrong place.

* Uploaded to wrong place.

* Re-save with Unix line endings

* Same for the header

* Commented out more code to fix all known text selection and highlighting problems.

* This fixes Mudlet#612 .

* Altered unhighlight() y1 declaration and assignment as requested. Also deleted 6 lines which were commented out.

* Removed all the scrollbar changes for issue Mudlet#612. This now only handles text selection, and scroll bar changes are in a separate branch.

* Fixed drawing highlight box for double clicking a word.

* Altered ctrl click to select line on single ctrl click (Previously you needed to move the mouse to select the line(s).

* Unix line endings again

* Raise code to formatting standard

(use the clang-format tool with the .clang-format file at the root of the repository for this)

* Fixed ctrl-drag selection. Ensured ctrl-click selection worked too.

* Unix line endings
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.

Copying Text Issue
5 participants