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

Copy solely the text string when TextEdit is active (DnD). #499

Merged

Conversation

tobiolo
Copy link
Collaborator

@tobiolo tobiolo commented Sep 1, 2023

Same as 7f68776 but for Drag and Drop operation.

@aardappel
Copy link
Owner

That seems very similar code then.. should that be pulled into a function?

@tobiolo tobiolo force-pushed the copy-only-text-in-textedit-mode-dnd branch from e59b37a to 8ff3e99 Compare September 1, 2023 18:25
@tobiolo
Copy link
Collaborator Author

tobiolo commented Sep 1, 2023

That seems very similar code then.. should that be pulled into a function?

This is a fantastic idea. I have merged the code for clipboard and drag and drop in into one main control flow.
What do you think?

@aardappel
Copy link
Owner

That required so many conditionals, that is definitely worse :)

What I meant is that I saw this code being identical in 2 places:

                        sys->clipboardcopy = s;
                        wxString html = selected.g->ConvertToText(selected, 0, A_EXPHTMLT, this);
                        auto *htmlobj = 
                        #ifdef __WXGTK__
                            new wxCustomDataObject(wxDF_HTML);
                        htmlobj->SetData(html.Len(), html);
                        #else
                            new wxHTMLDataObject(html);
                        #endif

So that could possibly be in a function, and maybe other parts of the code too.

@tobiolo tobiolo force-pushed the copy-only-text-in-textedit-mode-dnd branch from 8ff3e99 to 7d0dafa Compare September 1, 2023 20:03
@tobiolo
Copy link
Collaborator Author

tobiolo commented Sep 1, 2023

That required so many conditionals, that is definitely worse :)

What I meant is that I saw this code being identical in 2 places:

                        sys->clipboardcopy = s;
                        wxString html = selected.g->ConvertToText(selected, 0, A_EXPHTMLT, this);
                        auto *htmlobj = 
                        #ifdef __WXGTK__
                            new wxCustomDataObject(wxDF_HTML);
                        htmlobj->SetData(html.Len(), html);
                        #else
                            new wxHTMLDataObject(html);
                        #endif

So that could possibly be in a function, and maybe other parts of the code too.

@aardappel
I agree. What about this iteration then? :-)

@aardappel
Copy link
Owner

I am not sure what you're asking.

@tobiolo
Copy link
Collaborator Author

tobiolo commented Sep 2, 2023

I am not sure what you're asking.

Ok. I updated the PR (see commit). Is it ok now?

src/document.h Outdated
@@ -496,6 +480,23 @@ struct Document {
return;
}

#ifdef __WXGTK__
Copy link
Owner

Choose a reason for hiding this comment

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

why did you split it up by duplicating the code? the old code was better

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The return types differ between WXGTK and other platforms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok I changed that. But I need to set the return type and differentiate between WXGTK and others as also within the function body, there is a second differentiation then.

@tobiolo tobiolo force-pushed the copy-only-text-in-textedit-mode-dnd branch 2 times, most recently from d01b8ef to c939f14 Compare September 2, 2023 15:57
@tobiolo
Copy link
Collaborator Author

tobiolo commented Sep 4, 2023

@aardappel
Is it ok now?

@tobiolo tobiolo force-pushed the copy-only-text-in-textedit-mode-dnd branch from c939f14 to 1e88aff Compare September 4, 2023 17:06
@tobiolo tobiolo requested a review from aardappel September 4, 2023 19:22
src/document.h Outdated
@@ -496,6 +480,22 @@ struct Document {
return;
}

#ifdef __WXGTK__
Copy link
Owner

Choose a reason for hiding this comment

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

just use a -> auto return type

Copy link
Collaborator Author

@tobiolo tobiolo Sep 5, 2023

Choose a reason for hiding this comment

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

And put the helper function above the caller did the "trick"!
Thanks for your feedback! :-)

src/document.h Outdated
sys->clipboardcopy = s;
wxString html = selected.g->ConvertToText(selected, 0, A_EXPHTMLT, this);
#ifdef __WXGTK__
wxCustomDataObject *htmlobj = new wxCustomDataObject(wxDF_HTML);
Copy link
Owner

Choose a reason for hiding this comment

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

auto *htmlobj = outside the ifdef like the original.. and indent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your reply. I also left the asterik now out because it is also deduced that it is a pointer type.

@tobiolo tobiolo force-pushed the copy-only-text-in-textedit-mode-dnd branch from 1e88aff to 5e85c75 Compare September 5, 2023 05:01
@tobiolo tobiolo requested a review from aardappel September 5, 2023 05:05
@tobiolo
Copy link
Collaborator Author

tobiolo commented Sep 6, 2023

@aardappel
Does the revision fulfill your requirements?

@aardappel
Copy link
Owner

Much nicer, thanks!

@aardappel aardappel merged commit 8d4073d into aardappel:master Sep 7, 2023
4 checks passed
@tobiolo
Copy link
Collaborator Author

tobiolo commented Sep 7, 2023

Much nicer, thanks!

Thank you for your review making it much better!

@tobiolo tobiolo deleted the copy-only-text-in-textedit-mode-dnd branch September 9, 2023 07:59
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.

2 participants