Just tested the patch and I think it's a good idea!
A few things though:

• The operator settings should be exposed as tool settings instead

• When the sampler is activated in another tool by holding the modifier key it should already indicate that in the cursor display.
• These same changes should also be added in parallel to the paint mode of the image editor

I'm not sure how in line with the general design of the tools in Blender the current use of an operator for this tool is.

In D9441#274245, @Simon Thommes (simonthommes) wrote:

Just tested the patch and I think it's a good idea!

@Simon Thommes (simonthommes) Thanks for the testing and feedback!!

A few things though:

• The operator settings should be exposed as tool settings instead

• Yeah I think so...

• When the sampler is activated in another tool by holding the modifier key it should already indicate that in the cursor display.

• It doesn't change? I will checkout cause that feedback is REALLY needed on this tool.

• These same changes should also be added in parallel to the paint mode of the image editor

Yeah sure, the reason I only added it to the 3d viewport is that to confirm before that having the operator exposed as a tool is a must so that I can add it to the Image Editor as well.

I'm not sure how in line with the general design of the tools in Blender the current use of an operator for this tool is.

We already have similar tools as in weight painting to sample the weight, I guess this case will be enough in-line with it

Thanks for the response!

In D9441#274685, @Juanfran Matheu (jfmatheu) wrote:

• When the sampler is activated in another tool by holding the modifier key it should already indicate that in the cursor display.

It doesn't change? I will checkout cause that feedback is REALLY needed on this tool.

It did only when holding the mouse button, when I tested. But I think it should already be indicated before pressing the mouse button, when the key is held.

• These same changes should also be added in parallel to the paint mode of the image editor

Yeah sure, the reason I only added it to the 3d viewport is that to confirm before that having the operator exposed as a tool is a must so that I can add it to the Image Editor as well.

Great! Yes, I was suspecting that.

I'm not sure how in line with the general design of the tools in Blender the current use of an operator for this tool is.

We already have similar tools as in weight painting to sample the weight, I guess this case will be enough in-line with it

I personally don't have a problem with it. I think when the mentioned changes are made it will be good to add to master. But the UI team should maybe have a look.

In D9441#274733, @Simon Thommes (simonthommes) wrote:

It did only when holding the mouse button, when I tested. But I think it should already be indicated before pressing the mouse button, when the key is held.

Got it, though I'm still not sure the keymaps allow me to call it without some not-modifier key, that's quite a limitation... And I think that's probably why the eyedropper is in the 'S' key actually. But I will give it a try.
I personaly like pressing a modifier key (more accessible and discoverable for a typical action when you texture paint) and pick the color directly without having to click to confirm cause it's more precise (especially when talking about tablet that is a must to paint) and to work fast when you are blending nearest colors.

I personally don't have a problem with it. I think when the mentioned changes are made it will be good to add to master. But the UI team should maybe have a look.

Yes, it will need UI team approval.

Thanks for addressing my feedback!

In D9441#277252, @Juanfran Matheu (jfmatheu) wrote:

• Now Alt shortcut triggers directly without clicking which is convenient and quicker.

That's actually something I am personally not a big fan of. I don't think a key like Alt should have an operator linked to it on its own. I'm also thinking of tablet users. Also, I think it is much more convenient and precise to make your selection with a click/tap than hovering with the mouse/pen and using the keyboard to select.
Maybe that is a thing of preference, but I feel like for tablet use it's just plainly better.
I was imagining it the same way it was working before, but already indicating the sample mode with a cursor change while alt is pressed. That would work more like temporarily entering the sample tool by holding down the key.
I personally think that would be more in line with the rest of the tool.

Note: couldn't add the operator properties there in the header or tool panel, it's more like a redo operator than a modal/interactive one.

Hmm, that's unfortunate. I guess this would need the operator to be implemented completely new with the tool usage in mind?
Would it at least be possible to remember the settings, so that they don't reset to default every time?

Speaking of the default settings, I think it should not add the color to a palette by default. That can be done explicitly afterwards.

Other than that I have a couple of remarks:

• There is still the brush-size indicator around the eyedropper icon when the tool is active. That's probably not needed here as it is more confusing. Ideally this would indicate a sample radius like can be used in the view mode, but I recognize that that's out of scope for this patch.
• alt-tabbing gets me perpetually stuck in the sampling mode until pressing alt again. Not sure how easy that is to prevent but pressing tab should probably just abort the sampling state.

In general:
I do still feel like it would be great to have a more generalized solution that merges together the functionality of the 3 different types of color samplers we have in Blender afaik. But in terms of discoverability this patch is already miles ahead of what we have currently.

In D9441#277317, @Simon Thommes (simonthommes) wrote:

I was imagining it the same way it was working before, but already indicating the sample mode with a cursor change while alt is pressed. That would work more like temporarily entering the sample tool by holding down the key.

Note: couldn't add the operator properties there in the header or tool panel, it's more like a redo operator than a modal/interactive one.

Hmm, that's unfortunate. I guess this would need the operator to be implemented completely new with the tool usage in mind?

Yeah, I wonder that the problem here is the operator is executed with the keymap we have {Alt key}.
Alt key will execute the operator and it will set the eyedropper cursor and hide the brush cursor (the circle).
In the case of the tool itself, works differently, it triggers with Leftclick but still we can set a tool cursor even if the operator is not being called.

Maybe with some operator property that acts as a flag could work... like 'Wait for click input' so that if it starts with Alt key (we'll have cursor!) and if that flag is set it won't pick colors until you do some left mouse clicking.
I will give it a try later.

• There is still the brush-size indicator around the eyedropper icon when the tool is active. That's probably not needed here as it is more confusing. Ideally this would indicate a sample radius like can be used in the view mode, but I recognize that that's out of scope for this patch.

Yeah I noticed it too but I think that's a bug in the image paint editor tools cause it can affect other tools that don't need the brush cursor.

• alt-tabbing gets me perpetually stuck in the sampling mode until pressing alt again. Not sure how easy that is to prevent but pressing tab should probably just abort the sampling state.

Keymap conflict... In my case Alt+Tab is for Windows's window manager. I don't have a solution for that combo now except removing that shortcut or returning the alt+leftmouse.