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

Refactor Clipboard methods #5627

Merged
merged 17 commits into from
Oct 10, 2020
Merged

Conversation

IanCaio
Copy link
Contributor

@IanCaio IanCaio commented Aug 12, 2020

This draft's goal is to refactor some of LMMS's clipboard functionality. Currently the system is a little bit unorganized because it mixes the OS's clipboard (accessed through QClipboard) and an internal LMMS's Clipboard class (only accessible on the current running instance of LMMS). It would be interesting to move completely to the OS's clipboard for the sake of consistency and also possibility of multiple LMMS instances interacting with themselves when it comes to copying and pasting.

The first commit is just a little reorganizing of some Clipboard related methods. First, the mime type string used to be obtained on both the Clipboard class and the StringPairDrag class (Clipboard::mimeType() and StringPairDrag::mimeType()). Since it's related to the clipboard, I moved it completely to the Clipboard class, the equivalents of the previous methods now being Clipboard::mimeType( Clipboard::Default ) and Clipboard::mimeType( Clipboard::StringPair ). Any new mime type can be easily added to the Clipboard class. The idea is that the Clipboard class will be a class with helper methods for dealing with the clipboard (not a clipboard itself as it is now). Another change is that StringPairDrag class had methods for decoding a string pair key and value from mime data and from a Drop event. I moved the ones that decode the key and value from mime data to the Clipboard class, since they can and are used outside the context of a Drag&Drop event, and kept the ones that extract the key and value from a Drop event on the StringPairDrag class (which by itself calls the Clipboard::decodeKey and Clipboard::decodeValue respectively).

I know the draft is at its very beginning, but being a structural change I think it's worth having it here so changes are discussed before I dive too deep into it. Suggestions can be made and I take a smaller risk of refactoring in a way that is not agreed upon by other Devs.

Starts moving logic that belongs to the Clipboard class to it. For now, that only includes the mimeType getter method (for all available mime types) and string pair decoding helper methods. Updated all files that uses those methods.

Removed a '#include "StringPairDrag.h"' line from a file that didn't use it at all (src/tracks/Pattern.cpp).

TODO:
	- Remove the second argument of the Clipboard::decodeKey and Clipboard::decodeValue methods, since they are only applicable for Clipboard::StringPair data.
	- Change Clipboard::copy and Clipboard::paste to use the QClipboard.
@LmmsBot
Copy link

LmmsBot commented Aug 12, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://9363-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-743%2Bg6e013cf-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9363?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://9364-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-743%2Bg6e013cf02-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9364?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://9361-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-743%2Bg6e013cf02-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9361?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/5dd7dqku36867wm3/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35570324"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dq7mmmpimv1gvss6/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35570324"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://9362-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-743%2Bg6e013cf02-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9362?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "edb06d7b016250775681005368dcba3787a3f168"}

Removes unnecessary mime type argument to those methods since they are only used on data of StringPair type, so the mime type will always be Clipboard::StringPair.
	This commit adds a method to the Clipboard class that takes a key and a value as arguments and copies a string pair to the clipboard, moving the logic of actually copying to the Clipboard class. That simplifies its use on the Track.cpp methods and better distribute the responsabilities between classes.

	Another change was calling TCOV::copy() from the TCOV::cut() method instead of repeating the code, saving some unnecessary lines.
	This commit adds a method to the Clipboard class to copy a generic string to the clipboard, using the given MimeType from our enum. That is used in the PianoRoll to copy notes, saving a few lines on PianoRoll.cpp by moving some of the clipboard logic to the Clipboard class.
@IanCaio
Copy link
Contributor Author

IanCaio commented Aug 16, 2020

Last few commits continue to move clipboard logic to the Clipboard class (Track.cpp and PianoRoll.cpp now have fewer lines wasted with setting up the mime data and other things that are now made easier by helper methods from Clipboard). Also, saved a few lines on TrackContentObjectView::cut() by using TrackContentObjectView::copy() inside it to avoid duplicating code.

Next step is to figure out how to copy a QDomElement to the clipboard and retrieve it later, so I can convert the current Clipboard::copy and Clipboard::getContent methods to use the system's clipboard instead. Still not sure whether I should find a way to convert the QDomElement to a string and later rebuild the node using the copied text, or if I should find a way to copy the raw data to the clipboard and later retrieve it to build the node.

	Adds convenience methods to Clipboard.h to retrieve the QMimeData from the clipboard and checking whether a particular MimeType format is present on it. This saves a few lines on other files and reduces code pollution by moving more of the clipboard logic to the Clipboard class.
	To keep consistency on the behavior of the TCOV copy and paste operations, we now only use the TCOV::copy() and TCOV::paste() methods for copying both individual and multiple TCOs. The TCO::copy() and TCO::paste() methods are still kept since they are used on the src/track/BBTrack.cpp file.

TODO:
	- Since the TCO::copy() and TCO::paste() are now only used for internal operations that have nothing to do with Userland clipboard operations, I believe there's no need to change them to use the system's clipboard (if they do, cloning a BB Track would clear the clipboard which is a somewhat undesired behavior). However, if they don't use the clipboard they shouldn't be relying on the Clipboard.h class at all, so a small refactor would have to be done to move away from it. I believe renaming would also be interesting since now their behavior resembles more a copyState and pasteState operation.
	TCO::cut() is not being used anywhere anymore, so it's removed.
	TCO::copy() and TCO::paste() were until now only used to copy a state from a TCO to another one when cloning BBTracks. Being an internal operation there's no need for it to use the Clipboard. Also, the naming can be a little confusing now giving it's new only function. Both methods were merged in a single static one called TrackContentObject::copyStateTo(src, dst), which copies the state of one TCO to another one by using the same logic as before but without the need of the Clipboard class. BBTrack.cpp was updated to use this method instead and TCO::copy() and TCO::paste() were removed. Obsolete members of the Clipboard class were removed as well.
@IanCaio
Copy link
Contributor Author

IanCaio commented Aug 16, 2020

I'm changing this PR from Draft to Ready to review because the goal of the PR was achieved on a preliminary stage, which was refactoring the Clipboard class and clipboard operations so they are more consistent and all use the system's clipboard.

  • class Clipboard is now a helper class for dealing with the system's clipboard.
  • The TCO view copy and paste operations now use the same logic for either individual or multiple TCOs.
  • TCO::copy() and TCO::paste() methods were merged in a single TCO::copyStateTo( src, dst ) method, since the only place they were used after refactoring had this as a goal (src/tracks/BBTrack.cpp). Since it's an internal operation, not related to the userland copy/paste, for that method the clipboard is not used (if it were we would have unwanted behavior, as cleaning the clipboard when cloning a BBTrack).
  • TCO::cut was removed since it was not used anywhere after refactoring.
  • Tried to move a lot of the Clipboard logic to the class Clipboard to make the code on other files less polluted and more readable.

Right now, TCO::copyStateTo() converts src and dst from TrackContentObject * to JournallingObject * because that's how they were treated back when Clipboard::copy() and Clipboard::getContent() were used. I'm not sure if this is necessary so that's a point that could be addressed during review (not sure because I don't know if nodeName and saveState/restoreState could have different unwanted behaviors on their overridden versions).

For Testers:

Things worth testing:

  • Copy, Cut, Paste, Remove and Mute operations on individual TCOs, multiple TCOs and individual TCOs when there are other TCOs selected.
  • Cloning BBTracks with and without TCO content in their tracks.
  • Copy, Cut and Paste operations on other elements besides TCOs, like PianoRoll notes, automation values, text, etc.

@IanCaio IanCaio marked this pull request as ready for review August 16, 2020 15:23
@PhysSong
Copy link
Member

Right now, TCO::copyStateTo() converts src and dst from TrackContentObject * to JournallingObject * because that's how they were treated back when Clipboard::copy() and Clipboard::getContent() were used. I'm not sure if this is necessary so that's a point that could be addressed during review (not sure because I don't know if nodeName and saveState/restoreState could have different unwanted behaviors on their overridden versions).

Child classes are supposed to override nodeName(). No classes which inherit JournallingObject override saveState/restoreState. If you REALLY want to achieve that behavior, you should use src->JournallingObject::saveState() instead of casting.

@IanCaio
Copy link
Contributor Author

IanCaio commented Aug 17, 2020

Child classes are supposed to override nodeName(). No classes which inherit JournallingObject override saveState/restoreState. If you REALLY want to achieve that behavior, you should use src->JournallingObject::saveState() instead of casting.

To be honest, I wasn't sure if calling the base class nodeName() was necessary. I just did it that way because the original method operated over JournallingObjects. But taking another look at the code base I think it's not even a requirement to do the casting. Maybe someone can confirm, but I'll rewrite the method tomorrow morning to use TrackContentObject * instead and see how it behaves. I'm more confident right now that it will likely behave just the same.

	On the method TrackContentObject::copyStateTo the arguments were being casted to JournallingObject pointers unnecessarily, when they could have just be treated as TrackContentObject pointers resulting in the same behavior. This was being done because the original method (Clipboard::copy and Clipboard::getContent) received JournallingObject pointers as arguments, but that was probably done so they would work for other objects besides TCO. Since this new method only treats TCOs there's no need to do such conversion.
@IanCaio
Copy link
Contributor Author

IanCaio commented Aug 17, 2020

It was a quick fix, so I just updated the PR. Now the TrackContentObject::copyStateTo method doesn't cast the pointers to JournallingObject *. I made a quick test cloning some BBTracks that had tracks with TCOs and it seems to be working just the same. JournallingObject * was probably being originally used because Clipboard::copy and Clipboard::getContent were meant to be more generic and available to be used on other Journalling Objects besides TCOs.

Copy link
Member

@PhysSong PhysSong left a comment

Choose a reason for hiding this comment

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

There are minor formatting issues from copy-pasting. The code looks fine.
If someone can also review this, it will be great.

include/Clipboard.h Outdated Show resolved Hide resolved
include/Clipboard.h Outdated Show resolved Hide resolved
@DomClark DomClark self-requested a review September 10, 2020 14:22
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

I think Clipboard.cpp should be moved from src/core to src/gui. I don't know whether this should be done in this PR, or left for the refactoring effort.

include/Clipboard.h Outdated Show resolved Hide resolved
include/Clipboard.h Outdated Show resolved Hide resolved
src/core/Track.cpp Outdated Show resolved Hide resolved
src/core/Track.cpp Outdated Show resolved Hide resolved
src/core/Track.cpp Outdated Show resolved Hide resolved
@Veratil
Copy link
Contributor

Veratil commented Sep 11, 2020

I think Clipboard.cpp should be moved from src/core to src/gui. I don't know whether this should be done in this PR, or left for the refactoring effort.

💯% agree.

	This commit adds a new method to the Clipboard class, fixes some code style issues and improves the logic of a Track.cpp method. The changes are listed below:

	- A Clipboard::getString(MimeType) method is created, that returns a QString with the contents of the clipboard for that particular mimetype. PianoRoll.cpp was the only place where getMimeData() was still being used for that, so the code was changed to use this new helper method.
	- Removed parenthesis from return statements on Clipboard.h
	- On TrackContentObject::copyStateTo, the conditional for the state copy was moved to surround the whole method, since nothing is supposed to run if the conditional is false.
	- Removed comment that had a doubt about TCOV->remove(), since the doubt was answered on GitHub.

TODO:
	- AutomatableModelView.cpp also uses the clipboard, but right now it uses Qt methods directly. Maybe change the code to use the Clipboard class for consistency.
	Instead of calling Qt clipboard directly, AutomatableModelView.cpp now uses the Clipboard class to copy and paste values.
@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 12, 2020

I merged master and added 2 new commits. First one addresses the requested changes from the reviews listed below:

  • Removes parenthesis from return statements
  • Adds a getString method to Clipboard and change PianoRoll.cpp to use it instead of directly accessing the mime data.
  • Moves a conditional from TrackContentObject::copyStateTo to surround the whole method, since nothing has to be run if the conditional is false.
  • Removes a comment that had a question about TCOV->remove(), already answered by Dom.
  • Removes unnecessary == true in a conditional.

Second commit was a small change to AutomatableModelView.cpp. It used the Qt clipboard directly to copy float values to the clipboard and paste them back to an Automatable Model. It now uses the Clipboard class and the getString and copyString methods for consistency.

@PhysSong
Copy link
Member

There's a merge conflict due to #5619, but it looks easy to resolve that.

@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 13, 2020

There's a merge conflict due to #5619, but it looks easy to resolve that.

Yeah, from what I've seem the only thing I changed on this file was removing #include "StringPairDrag.h", which #5619 did too. I'll merge master here and update the PR to resolve the conflict.

	Resolves the conflict on src/tracks/Pattern.cpp caused by LMMS#5619. On this PR the only change to the file was removing an unnecessary include of the header "StringPairDrag.h", which the PR LMMS#5619 also did. So the file was kept the same way as in the master branch.
@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 13, 2020

Merged master and conflict on src/tracks/Pattern.cpp resolved!

	Changes the code to use enum class instead of a regular enum for the MimeType values.
@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 15, 2020

Changed the Clipboard class to use enum class instead of enum for the MimeType.

During the process a small bug was found. Steps for reproducing:

  1. Open the default project (TripleOsc, SampleTrack, BBTrack and AutomationTrack).
  2. Create 2 TCOs on the TripleOsc track on Bars 1 and 2
  3. Ctrl+Drag the TripleOsc track to the BBEditor
  4. Add some notes to the Kicker track (not necessary for the bug to happen)
  5. Clone the BBTrack

After those steps the notes added to the Beat/Bassline 0 Kicker track are gone and when you switch to the Clone of Beat/Bassline 0 track on the BB Editor the place where you add the notes to the Kicker are missing.

I'm still investigating what is causing it. Before, cloning a BBTrack would trigger calls to TCO::copy() and TCO::paste(). On this PR the new method TCO::copyStateTo() is called instead. I still haven't found reasons for that method to cause this misbehavior but I'll keep debugging here.

@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 16, 2020

About the bug mentioned before: Apparently it was already present before my PR, I'm sorry about that. I'll still try to figure out what is causing it, but disregard that bug when reviewing the code.

Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

Looks good to me. One more thing: how about making Clipboard a namespace instead of a class, since everything in it is now public and static?

@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 16, 2020

Sure, it could be done! I'd basically have to replace class by namespace and remove public: and static keywords right? I tried quickly here but ran into some linker issues with multiple declarations, will have to take a look with more attention later.

Edit:
I meant besides adding using namespace Clipboard on the methods where the functions are being used so I could remove Clipboard::.

@PhysSong
Copy link
Member

If you run into multiple definitions issue, you can add inline/static inline or move the definition to the source file depending on the function body.
inline without static means "select any of multiple copies", and static means "use the local copy, and don't care about other copies".

	Makes Clipboard a namespace instead of a class and adds "using namespace Clipboard;" statements to methods that required functions from that namespace. The only two methods where the statement wasn't added were StringPairDrag::decodeKey and StringPairDrag::decodeValue because they are both one-liners.
@IanCaio
Copy link
Contributor Author

IanCaio commented Sep 17, 2020

Clipboard was changed from a class to a namespace, and using namespace Clipboard; statements were added where needed (a comment above the statement with the used methods/variables from the namespace were also added). There were two methods where I didn't add the using namespace, which were StringPairDrag::decodeKey and StringPairDrag::decodeValue since both were one-liners.

Thanks @PhysSong for the hint about using inline to avoid the multiple definition errors!

@IanCaio IanCaio requested a review from PhysSong October 5, 2020 01:19
@IanCaio
Copy link
Contributor Author

IanCaio commented Oct 5, 2020

Resolved the conflict introduced by #5661 . Does it look good to go?

On the "Files Changed" there appeared some stuff about src/3rdparty/ringbuffer, even though I ran git submodule update on that branch too. Is that normal?

@PhysSong
Copy link
Member

PhysSong commented Oct 5, 2020

I think you've commited the merge commit before running git submodule update. If you have troubles with submodules, I or some other devs may fix it.

@IanCaio
Copy link
Contributor Author

IanCaio commented Oct 5, 2020

I think you've commited the merge commit before running git submodule update. If you have troubles with submodules, I or some other devs may fix it.

Thanks! I thought I ran it before the git merge. I'm unsure now because of the merge conflict, whether I ran it before git merge or before git commit. Anyways, even running it again now it doesn't show any changes to be committed, only on the Files Changed here on Github. Do I have to do anything to fix that?

@PhysSong
Copy link
Member

PhysSong commented Oct 5, 2020

In that case, ping me when it's ready to merge.

	Fixes conflict introduced by LMMS#5661 ("Fix for Mixer volume percentage labels are off by a factor of 100") on src/gui/AutomatableModelView.cpp.

	Note: This merge was force-pushed after a hard reset, because there were some issues with submodules being included on the last time I pushed the merge. That new merge is an attempt to fix that.
@IanCaio IanCaio force-pushed the feature/clipboardRefactor branch from ed3777f to edb06d7 Compare October 5, 2020 10:21
@IanCaio
Copy link
Contributor Author

IanCaio commented Oct 5, 2020

I think I fixed it. Did a hard reset to revert the merge and did it again. Even running git submodule update before the merge, the src/3rdparty/ringbuffer folder appeared as "modified but not staged". Last time I think I accidentally added it to the commit. This time I left it out, finished the merge and ran git submodule update again after it. The ringbuffer folder was then updated correctly. So maybe git submodule update takes into account the current commit timestamp and the first git submodule update kept ringbuffer on the state it was on my last commit in this branch. Because master had updated files for ringbuffer, it caused them being shown as "modified but not staged" on the merge. Keeping them outside the merge and then running git submodule update again probably updated ring buffer to the current timestamp of master that was just merged to this branch making them match again.

Well, that's just what I think happened. Anyways, the ringbuffer files are out of the "Files Changed" preview now!

@PhysSong
Copy link
Member

PhysSong commented Oct 8, 2020

Merge?

@PhysSong PhysSong changed the title Refactoring of Clipboard methods Refactor Clipboard methods Oct 8, 2020
@PhysSong PhysSong merged commit b64fe8e into LMMS:master Oct 10, 2020
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Moves mimeType from StringPairDrag to Clipboard

* Simplifies decodeKey and decodeValue methods

* Adds method to copy a string to clipboard

* Adds method to copy a string pair to the Clipboard

* Adds convenience methods to Clipboard.h to retrieve the QMimeData from the clipboard and checking whether a particular mime type format is present on it

* Uses only the TCOV copy/paste methods on the song editor
To keep consistency on the behavior of the TCOV copy and paste operations, we now only use the TCOV::copy() and TCOV::paste() methods for copying both individual and multiple TCOs.

* Removes obsolete TrackContentObject::cut() and merge copy() and paste() methods to single function TrackContentObject::copyStateTo()

* Adds Clipboard::getString method to get data for particular mime type

* Makes AutomatableModelView.cpp use the Clipboard class instead of calling Qt clipboard directly
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.

5 participants