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

channelmixerrgb: remember last selected notebook page #6793

Merged
merged 2 commits into from
Nov 8, 2020

Conversation

elstoc
Copy link
Contributor

@elstoc elstoc commented Nov 8, 2020

No description provided.

@TurboGit TurboGit added this to the 3.4 milestone Nov 8, 2020
@TurboGit TurboGit self-requested a review November 8, 2020 16:29
@TurboGit TurboGit added feature: enhancement current features to improve scope: UI user interface and interactions labels Nov 8, 2020
@@ -1915,6 +1915,9 @@ void gui_init(struct dt_iop_module_t *self)
NOTEBOOK_PAGE(grey, grey, N_("grey"), N_("grey"), FALSE)

self->widget = GTK_WIDGET(g->notebook);
int active_page = dt_conf_get_int("plugins/darkroom/channelmixerrgb/gui_page");
Copy link
Member

Choose a reason for hiding this comment

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

const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I copied this from tone equalizer, so have added a const there as well.

@TurboGit
Copy link
Member

TurboGit commented Nov 8, 2020

BTW, I'm not sure this is good. When I use this module (or previous channel mixer BTW), when I change the R channel I do not expect to restart here for next edit. So not really convince this is helpful.

@elstoc
Copy link
Contributor Author

elstoc commented Nov 8, 2020

I don't want to start in the CAT tab either, because I mostly use it for monochrome conversion

@ptilopteri
Copy link

make it a preference within the module

@johnny-bit
Copy link
Member

I think this is in the same vein as recent toneeq change, where last used tab is remembered. It's imo good practice for multi-tab modules, as it gives good workflow by default for those that are tab-independent (like toneeq and channelmixer). I wouldn't recommend remembering tabs on filmic though ;)

@TurboGit
Copy link
Member

TurboGit commented Nov 8, 2020

Ok, I let's go with that and we"ll see how people react. Note that I'm not for a preference as we already have many, or if a preference is to be added it should be applied to ALL modules.

@TurboGit TurboGit merged commit fbdf363 into darktable-org:master Nov 8, 2020
@matt-maguire
Copy link
Contributor

Using channel mixer for making monochrome images or for manually adjusting primaries are not common use cases, whereas CAT tab is relevent for all images. That's why I think it should be first and selected by default.

@elstoc
Copy link
Contributor Author

elstoc commented Nov 8, 2020

Back to that discussion about the assumptions that the module is making. It should either be a no-op when activated or it should somehow set the white balance module to D65 (or I'm misunderstanding this whole thing still).

@rabauke
Copy link
Contributor

rabauke commented Nov 8, 2020

I used channel mixer rgb for the 1st time today. I think it's great. But two things are confusing (at least at the beginning):

  • The module combines the functionality of a "classic" channel mixer with white balancing.
  • The default setting represents not a no-op. (Source should be "same as pipeline (D50)")

@elstoc
Copy link
Contributor Author

elstoc commented Nov 8, 2020

@rabauke: exactly.

PR #6782 should partially resolve this so that people aren't expecting a 'traditional' channel mixer. There is still some work needed to integrate this module into a reasonable workflow (namely how to ensure that the WB module is set to D65 when the CAT tab is used).

@parafin
Copy link
Member

parafin commented Nov 8, 2020

Not all modules have default settings resulting in no-op, so it's not a rule per se. The question is how particular module is usually used, if returning to no-op settings is something that people want to do often, then it makes sense to change defaults I guess.

@dterrahe
Copy link
Member

I think this is in the same vein as recent toneeq change, where last used tab is remembered.

I rather belatedly realise now (while chasing down a crash for @AxelG-DE in midi) that this is somewhat problematic for multi-instance (more conceptually rather than implementation). The last gui_cleanup called will set the saved tab. So if different tabs are active in several instances, the saved state might be unpredictable. Could be (I haven't experimented/verified this) that if you create a 2nd instance, change tabs there, then close it, that tab maybe gets saved. Now if you create another instance again, it would use that saved state, rather than whatever tab the instance you duplicate from is on.

Just to mention this and see if anybody has a simple obviously better (and working!) solution. I don't think this is critical; if you always want to use a particular tab that'll end up being the one that gets saved and restored anyway.

@elstoc elstoc deleted the channeltabs branch February 9, 2021 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: enhancement current features to improve scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants