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

Allows preview while styles disabled, fix #226 #254

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

Conversation

yfdyh000
Copy link
Contributor

No description provided.

@hideheader
Copy link

This doesn't work as expected.

  1. Enable an installed site style
  2. "Turn all styles off" - first style is disabled
  3. Preview a second site style - neither style is applied. I expect to see the second, Previewed style but not the first style.
  4. "Turn all styles on" - first style is applied, but not the second, Previewed style. Both styles should be applied.

I expect previewed styles to always be on, independent of "Turn all styles on/off". That permits me to see the effect of just one style that I'm editing or evaluating, without the previewed style interacting with other, installed styles.

@yfdyh000
Copy link
Contributor Author

@hideheader Can you try it again? I fixed the previewed style will remain to be applied, if turn off and just installed.

@hideheader
Copy link

OK, now I see
3. Preview a second style - second, Previewed style is applied.
4. "Turn all styles on" - both styles are applied.
5. "Turn all styles off", then "Install" - neither style is applied.
6. "Turn all styles on" - both styles are applied.

Previewing a style in the editor also behaves this way. Good job.

@hideheader
Copy link

When I edit and Preview a style, my expectation is that (1) the Preview is not affected at all by 'Turn all styles off (on)' and (2) the Preview is not affected at all if the (saved) version is disabled or enabled from the Stylish popup menu. Given that, I see two more problems.

First, while Previewing a style in the Stylish editor, 'Turn all styles on' displays the saved version, not the edited version which was Previewed. Here is a common use case:

  1. Write new style:
    @-moz-document url-prefix(https://userstyles.org) { * {color: blue !important} }
  2. Preview - blue text.
  3. Save. Change 'blue' to 'green'. Preview - green text.
  4. Turn all styles off, Turn all styles on - blue text. (Incorrect - that is the Saved style.)

The following demonstrates that the problem is with 'Turn all styles on':

  1. Write new style:
    @-moz-document url-prefix(https://userstyles.org) { * {color: blue !important} }
  2. Save - blue text.
  3. Turn all styles off - black text. (Correct - that is the unstyled color.)
  4. Change 'blue' to 'green'. Preview - green text.
  5. Turn all styles on - blue text. (Incorrect - that is the Saved style.)

Second, while Previewing a style in the editor, disabling the style from the popup menu disables the Previewed style. Here is a use case that demonstrates the behavior:

  1. Write new style:
    @-moz-document url-prefix(https://userstyles.org) { * {color: blue !important} }
  2. Save - blue text.
  3. Change 'blue' to 'green'. Preview - green text.
  4. Disable style - black text. (Incorrect - that is the unstyled color.)
  5. Enable style - blue text. (Incorrect - that is the Saved style.)

The odd Enable behavior doesn't depend on the odd Disable behavior:

  1. Write new style:
    @-moz-document url-prefix(https://userstyles.org) { * {color: blue !important} }
  2. Save - blue text.
  3. Disable the style - black text. (Correct - that is the unstyled color.)
  4. Change 'blue' to 'green'. Preview - green text.
  5. Enable the style - blue text. (Incorrect - that is the Saved style.)

@yfdyh000
Copy link
Contributor Author

yfdyh000 commented Dec 8, 2015

Hi @hideheader, thank you for writing such a detailed test cases.
I reproduce and understand some of the problems (problem 3 and 4), and I think they are not a big problem. Preview styles will not be persisted, it may be covered due to enable and disable, not until the editor window is closed.

@hideheader
Copy link

I'm not worried that the Preview will be persisted; I never saw that happen. I agree that this could be used as it is, but the Preview button in the editor should be re-enabled on 'Turn all styles on' so that it is consistent with the current display state. Rewriting my test cases,

  1. 'Preview': see the edited version + all other styles
  2. 'Turn all styles off': see only the edited version
  3. 'Turn all styles on': see the original version + all other styles
  4. 'Preview': ...

'Turn all styles on' has the side-effect "Un-preview" in the editor (state 1 vs state 3), which could be a new feature, not a bug, if the UI enables the "Preview" button so that it can be used again.

This "Un-Preview" behavior doesn't happen when there is no saved version of the Previewed style. Here is what happens when Previewing a new style in the editor, or Previewing a style from the Install dialog.

  1. 'Preview': see the new style + all other styles
  2. 'Turn all styles off': see only the new style
  3. 'Turn all styles on': see the new style + all other styles

(This is the behavior I expected of edited styles, too.) In this case the Preview button need not be re-enabled on 'Turn all styles on', but there is no harm in doing it. (Do you agree, @JasonBarnabe?)

@yfdyh000
Copy link
Contributor Author

I mean, preview state may be covered by any operations instead of continuing in the entire session.
Okay, I understand, preview an saved style should not be on/off effect?

@hideheader
Copy link

I think a Preview should always be on, but if a Preview is covered by another operation (as happens now) then the "Preview" button should be enabled so I can use it again.

I would even be satisfied with the current behavior if the Preview button was always enabled. (But ask Jason before you do it, because my opinion is worth very little here.)

Just to be clear, I am using this now. It's not exactly what I expected, but it's good. I have to "type a space+undo" to get my "Preview" button back, though, after 'Turn all styles on'.

@hideheader
Copy link

OK, from time to time now I'm seeing an old Previewed revision of a style I'm editing persist in the Stylesheet Service after a newer revision is Previewed ("zombie" styles). I think it may be an existing Preview bug, but I can't tell you yet exactly how to reproduce it.

Here's what I know about zombies:
[Bug] Zombie styles in Firefox (yes, they're back)
"Stylish for Firefox 2.0.0b4 :: Changes from 1.4.3 to 2.0.0b1 :: Bug Fixes"
#126

@JasonBarnabe
Copy link
Contributor

Without having tested or reviewed the code, I already have two issues with this:

  1. I understand why someone would want it to work like this, but I'm not sure that this is necessarily the expected behaviour for everyone.
  2. With the XPCOM-based Stylish going away soon, and the finicky nature of all this code, I'm not sure it's worth it to put in a release with this change.

I'm open to being convinced otherwise, but this my current opinion.

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.

None yet

3 participants