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.

Sign up for GitHub

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

Jump to bottom

Fix #15946: Add ability to replace image #15985

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix #15946: Add ability to replace image #15985

wants to merge 1 commit into from
+278 −11

Conversation

iwoithe
Copy link
Contributor

@iwoithe iwoithe commented Jan 20, 2023

Resolves: #15946

Video

mu-15975.mp4
  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@iwoithe iwoithe mentioned this pull request Jan 20, 2023
Open
@iwoithe iwoithe force-pushed the fix-15946-add-ability-to-replace-image branch from f18b6f3 to 2297c62 Compare January 20, 2023 05:21
Eism
Eism reviewed Jan 20, 2023
View reviewed changes
src/notation/internal/notationactioncontroller.cpp Outdated
return;
}

std::vector<std::string> filter = { trc("notation", "All Supported Files") + " (*.svg *.jpg *.jpeg *.png)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I see a code duplication problem.
If someone decides to make a change to this code(adding another filter) and forgets to make a change to addImage, then we will get a different behavior - a bug.

I propose to move the common parts into separate method(s) and reuse them in addImage and replaceImage methods.

Same for NotationInteraction::addImage and NotationInteraction::replaceImage

@bkunda
Copy link

bkunda commented Jan 20, 2023

From a functional/UX POV I think this is all very nice. I don't have any problems with this proposal 🙂.
Thanks again @iwoithe!

@iwoithe iwoithe force-pushed the fix-15946-add-ability-to-replace-image branch from 2297c62 to 4bbb486 Compare January 26, 2023 00:32
@iwoithe iwoithe force-pushed the fix-15946-add-ability-to-replace-image branch from 4bbb486 to 8ac6f31 Compare March 11, 2023 02:12
@bkunda bkunda mentioned this pull request Apr 19, 2023
Open
@iwoithe iwoithe force-pushed the fix-15946-add-ability-to-replace-image branch from 8ac6f31 to 8ab89e3 Compare May 10, 2023 22:24
Eism
Eism reviewed May 11, 2023
View reviewed changes
src/inspector/view/qml/MuseScore/Inspector/notation/images/ReplaceImageSettings.qml Outdated

navigation.panel: root.navigationPanel
navigation.name: "KeepSizeRadioButton"
navigation.row: root.navigationRowStart + 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Root doesn't have navigationRowStart property
I think it should be just navigation.row: 1

Eism
Eism reviewed May 11, 2023
View reviewed changes
src/notation/internal/notationinteraction.cpp Outdated
if (imagePath.empty() || !item) {
return;
if (imagePath.empty()) {
return new Image();
Copy link
Contributor

Choose a reason for hiding this comment

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

it is better to return nullptr

Eism
Eism reviewed May 11, 2023
View reviewed changes
src/notation/internal/notationinteraction.cpp Outdated
@@ -4179,14 +4178,24 @@ void NotationInteraction::addImageToItem(const io::path_t& imagePath, EngravingI

ImageType type = mu::value(suffixToType, suffix, ImageType::NONE);
if (type == ImageType::NONE) {
return;
return new Image();
Copy link
Contributor

Choose a reason for hiding this comment

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

it is better to return nullptr

Eism
Eism reviewed May 11, 2023
View reviewed changes
src/notation/internal/notationinteraction.cpp Outdated
}

Image* image = Factory::createImage(item);
image->setImageType(type);

if (!image->load(imagePath)) {
delete image;
return new Image();
Copy link
Contributor

Choose a reason for hiding this comment

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

it is better to return nullptr

@iwoithe
Copy link
Contributor Author

iwoithe commented May 11, 2023

@Eism Thanks for the review. 🙂

(apologies, forgot to mention this when pushing)
At the moment this PR is incomplete - I committed the current changes so I could deal with the other PRs. As such, there are still a few things left to do as stated in the TODOs and FIXMEs.

@iwoithe iwoithe marked this pull request as draft May 11, 2023 11:53
@iwoithe iwoithe force-pushed the fix-15946-add-ability-to-replace-image branch from 8ab89e3 to e96d002 Compare August 16, 2023 07:45
@bkunda
Copy link

bkunda commented Sep 14, 2023

@iwoithe are you still intending to complete this PR? 🙂

@iwoithe
Copy link
Contributor Author

iwoithe commented Sep 15, 2023

@bkunda Yes, I still intend to complete this PR. 🙂

@iwoithe
Copy link
Contributor Author

iwoithe commented Feb 5, 2024

FWIW I have resumed work on this PR, however, for various reasons (bugs, code quality, etc.) I have restarted from scratch and as such a new PR will be opened soon 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eism Eism Eism left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MU4 Task] Add functionality "Replace image"
3 participants
@iwoithe @bkunda @Eism