Nothing Special   »   [go: up one dir, main page]
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

PageView cleanup #4792

Merged
merged 5 commits into from
Apr 13, 2023
Merged

PageView cleanup #4792

merged 5 commits into from
Apr 13, 2023

Conversation

jhilmer
Copy link
Contributor
@jhilmer jhilmer commented Apr 7, 2023

No description provided.

jhilmer
jhilmer commented Apr 7, 2023
View reviewed changes
src/core/gui/PageView.cpp
Comment on lines -647 to -649
Layer* layer = this->page->getSelectedLayer();
const Layer::Index layerId = this->page->getSelectedLayerId();
const bool isBackgroundLayer = (layerId == 0);
Copy link
Contributor Author
@jhilmer jhilmer Apr 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Dead code ?? Are there any side effects of those lines? I think it safe to remove. layer, layerId, and isBackgroundLayer is not used.

Copy link
Member

Choose a reason for hiding this comment

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

They can be removed indeed. It looks like I forgot to remove those lines in #4673.

@jhilmer jhilmer force-pushed the pageview-cleanup branch 2 times, most recently from 823041d to 7a1a629 Compare April 8, 2023 12:24
@jhilmer jhilmer force-pushed the pageview-cleanup branch from 7a1a629 to 1a2940e Compare April 8, 2023 12:26
rolandlo
rolandlo approved these changes Apr 9, 2023
View reviewed changes
Copy link
Member
@rolandlo rolandlo left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick. Otherwise this looks good to me.

src/core/gui/PageView.cpp Outdated Show resolved Hide resolved
@jhilmer jhilmer requested a review from rolandlo April 11, 2023 21:54
rolandlo
rolandlo approved these changes Apr 11, 2023
View reviewed changes
Copy link
Member
@rolandlo rolandlo left a comment

Choose a reason for hiding this comment

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

Thanks @jhilmer !
Merging in 24 hours if no objections are raised.

@rolandlo rolandlo added the merge proposed Merge was proposed by maintainer label Apr 11, 2023
@bhennion bhennion merged commit 81c1a58 into xournalpp:master Apr 13, 2023
@Technius Technius added this to the v1.2.0 milestone Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolandlo rolandlo rolandlo approved these changes

Assignees
No one assigned
Labels
merge proposed Merge was proposed by maintainer
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jhilmer @rolandlo @Technius @bhennion