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

Fix #12459 : text selection drag and drop problem. #12460

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

labordep
Copy link
Collaborator

I propose this fix for #12459.
It is hard to reproduce with a test (complexe mouse events) but with this fix I haven't the bug.
Thanks.

@welcome
Copy link

welcome bot commented Jan 29, 2023

Thanks for opening this pull request! Now continious integration (CI) will build Pharo with your change and run all tests. This might fail due to many reasons! Please check if your PR breaks the build or makes tests fail. Feel free to add comments to the PR. After this, before your PR can be merged it needs one or more reviews. Do not hesitate to ask people (on the Mailinglist or Discord) to help! When the CI shows no problems and there are positive reviews, your PR will be merged.

@Ducasse
Copy link
Member

Ducasse commented Jan 29, 2023

Thanks Pierre!!!

Copy link
Member

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Thanks Pierre! I tested this on my side and it makes the bug go away.
I'd like if you can add some comments on why the arithmetic?

If you want to write a test, I suggest to take a look at RubSelectionTest

imagen

@labordep
Copy link
Collaborator Author

Hi,
Ok good news !
I will write unit test and comment and send a new PR soon :)

@labordep
Copy link
Collaborator Author

I have rewrite some code into the RubTextEditor with unit tests. The unit tests represent the search behavior that I suppose. The selection behavior on double click is not perfect but I think this is working better. Check my tests to have a reflexion about the targeted behavior.

@Ducasse Ducasse closed this Feb 5, 2023
@Ducasse Ducasse reopened this Feb 5, 2023
@Ducasse
Copy link
Member

Ducasse commented Feb 5, 2023

I do not get why suddenly we get many dispersed broken tests

@labordep
Copy link
Collaborator Author

labordep commented Feb 5, 2023

@Ducasse I have to do something ?

@MarcusDenker MarcusDenker reopened this Feb 9, 2023
@MarcusDenker
Copy link
Member

The PR seems to lead to these tests failing:

CompletionEngineTest

  • testReplaceTokenWithCaretInTheMiddleOfWordReplacesEntireWord
  • testReplaceTokenWithCaretOnEndOfWordReplacesEntireWord

ProperMethodCategorizationTest

  • testSetUpMethodInSUnitTestsNeedsToBeInRunningProtocol

ProtocolConventionsTest

  • testProperTestProtocolIsUsed

@MarcusDenker MarcusDenker added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Feb 9, 2023
@labordep
Copy link
Collaborator Author

Thanks @MarcusDenker, I will check theses tests

@labordep
Copy link
Collaborator Author

I have fixed a bad index computation in the completion engine and all theses tests are ok for me :

  • CompletionEngineTest
  • ProperMethodCategorizationTest
  • ProtocolConventionsTest
  • RubTextEditorTest

I'm waiting for global test checking

@labordep
Copy link
Collaborator Author

@MarcusDenker @Ducasse I don't know this system of auto verification, it appears very powerful. Apparently there is an error, how can I see where is the problem ?

@MarcusDenker
Copy link
Member

the CI checks on 3 platforms, but the windows test runner seems to be out of disk space, so it stopped.

else it looks good.

I will open an issue for the windows test runner and put this PR on the "ready to review" status

@MarcusDenker MarcusDenker added Status: Tests passed please review! and removed Status: Need more work The issue is nearly ready. Waiting some last bits. labels Feb 13, 2023
@guillep
Copy link
Member

guillep commented Feb 24, 2023

I've manualy fixed the merge conflict. I'll integrate after we check it runs and I did not break anything :)

@MarcusDenker MarcusDenker reopened this Mar 2, 2023
@MarcusDenker MarcusDenker merged commit c9c7ad4 into pharo-project:Pharo11 Mar 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bug when commentary text zone in the Class Browser is drag'n drop with selection
4 participants