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

hswidget/PaletteSelectWidget: refactor #312

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

gucio321
Copy link
Contributor

@gucio321 gucio321 commented Jun 7, 2021

This PR contains a refactor of palette select (for dc6, dcc and d61 editors) related code.

CHANGELOG:

  • fix DCC Editor "Change Palette" cannot be closed by the window's X button #299
    • fix bug fix bug, when palette selector wasn't able to change palette
  • remove unnecessary rgba field from dc6 widget state
  • decide, if palette should be saved in as part of editor state
  • rewrite dcc, dc6 and dt1 widget: palette shouldn't be an argument to Create. I suppose, it'd be better to add a separated method for setting this argument.

@gucio321 gucio321 changed the title hswidget/PaletteSelectWidget: fix #299 hswidget/PaletteSelectWidget: refactor Jun 9, 2021
@gucio321 gucio321 force-pushed the issue-299 branch 2 times, most recently from c5c280d to a581bd1 Compare June 13, 2021 09:06
@gravestench
Copy link
Contributor

please rebase off of upstream master

@gucio321
Copy link
Contributor Author

gucio321 commented Jul 4, 2021

@gravestench done

Copy link
Contributor

@gravestench gravestench left a comment

Choose a reason for hiding this comment

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

There's a bug, please fix.

Can not set palette for DC6 (your issue-299 branch):

Screen.Recording.2021-07-05.at.7.33.17.PM.mov

On master, the feature is working as expected:

Screen.Recording.2021-07-05.at.7.35.00.PM.mov

@gucio321
Copy link
Contributor Author

okey, after a small linvestigation, I found that the bug is caused by build method improvement:

if e.selectPalette {
      // build palette select
}

        dt1Viewer := dt1widget.Create(e.state, e.palette, e.textu
        e.Layout(g.Layout{
                dt1Viewer,
        })

so building widget also happens if selectPalette is active
IMO this solution is good and widgets should detect fact of chaning palette
I'll put the palette field in widgetState structure, but It'll require a small edits of 3 widgets. I'll work on it soon.

@gravestench
Copy link
Contributor

I think that your proposed changes, which are related to selecting a palette, would be more coherent inside of this refactor.

@gucio321
Copy link
Contributor Author

yeah, that's what I mean exactly, I'll work on it before this PR will be ready

@gucio321 gucio321 marked this pull request as draft July 18, 2021 08:18
gucio321 and others added 5 commits July 19, 2021 19:35
when palette was changed, the widget didn't detected it. now palette is saved in widgetState and widget detects if palette was changed.
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.

DCC Editor "Change Palette" cannot be closed by the window's X button
2 participants