Make WordPress Core

Opened 13 years ago

Closed 9 years ago

#19291 closed enhancement (maybelater)

Widgets move to Inactive after clearing all widgets from sidebars

Reported by: lancewillett's profile lancewillett Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: Widgets Keywords: needs-patch needs-testing
Focuses: Cc:

Description

Jane reported in IRC that a theme switch dropped her widgets to Inactive.

Jane: test site had 2010 as theme, with some widgets. activated 2011 on themes.php. Refreshed itself to themes.php?activated=true and showed alert message " New theme activated. This theme supports widgets, please visit the widgets settings screen to configure them." Go to widgets screen, widgets in primary sidebar in 2010 are not in sidebar, no widgets live anymore
Jane: switch back to 2010, same message appears on themes.php?activated=true and 2010 sidebar comes back

Reproduced by ocean90.

I was able to repeat the reported issue one only one specific case.

This only happens if you remove *all* widgets from all sidebars. Then add widgets back to sidebar(s). Then, switch back to a previously activated theme.

  • It doesn't happen if you leave at least one widget in place in a sidebar
  • It doesn't happen if you switch to a new theme (not previously activated)

Looks like we can solve it by the "lost" widget check just puts any old widgets into Inactive. We should orphan those, too.

To fix: If widgets registered but not in the new theme's sidebar, orphan it.

See #17979, #19091, #19092.

Attachments (2)

19291.patch (2.7 KB) - added by lancewillett 13 years ago.
19291.1.patch (1.4 KB) - added by lancewillett 13 years ago.
Refreshed my patch to remove changes in r19340.

Download all attachments as: .zip

Change History (31)

@lancewillett
13 years ago

#1 @aaroncampbell
13 years ago

  • Cc aaroncampbell added

#2 follow-ups: @lancewillett
13 years ago

Note, this patch includes http://core.trac.wordpress.org/attachment/ticket/19092/19092.2.patch — so relies on that being committed also.

#3 in reply to: ↑ 2 @azaozz
13 years ago

Replying to lancewillett:
19092.2.patch treats the previous theme's saved data as the "main" data which seems undesirable. The "main" data is the data from the previous theme, not the saved data from some time ago.

#4 in reply to: ↑ description ; follow-up: @azaozz
13 years ago

Replying to lancewillett:

This only happens if you remove *all* widgets from all sidebars.

This border case is interesting. When switching themes we do not save widgets instances/settings. We only save widgets positions in the sidebars.

If the user removes all widgets from all sidebars there will be no more widgets instances to match the old position settings when restoring an old theme's sidebars (the newly added widgets will be different instances with different settings). In that case IMHO we should be adding all of the newly added widgets to Inactive Widgets.

Apparently it may get even messier if the user reloads the page after removing all widgets from all sidebars. Then the newly added widgets would most likely have the same instance_IDs as the old/removed widgets but probably different settings and different positions. Restoring an old theme's sidebars in that case would match some/all of the newly added widgets and put them where the old instances used to be.

Last edited 13 years ago by azaozz (previous) (diff)

#5 in reply to: ↑ 4 ; follow-up: @lancewillett
13 years ago

Replying to azaozz:

Replying to lancewillett:
19092.2.patch treats the previous theme's saved data as the "main" data which seems undesirable. The "main" data is the data from the previous theme, not the saved data from some time ago.

This is what Aaron and I set out to do in #17979. We're treating the saved data as canonical, and moving anything else to Orphaned widgets.

So the patch is correct in that sense.

Replying to azaozz:

Replying to lancewillett:

This only happens if you remove *all* widgets from all sidebars.

This border case is interesting. When switching themes we do not save widgets instances/settings. We only save widgets positions in the sidebars.

In that case IMHO we should be adding all of the newly added widgets to Inactive Widgets.

Please, no. :)

We want to avoid moving anything to Inactive if possible. Remap to new sidebar is 1st priority, which works in all cases but this one.

Second priority is to move to Orphaned.

My goal is to not ever see anything in Inactive unless a user moves it there.

#6 @lancewillett
13 years ago

By the way, I'd consider this issue minor, and not a beta 4 blocker.

#7 in reply to: ↑ 5 ; follow-up: @azaozz
13 years ago

Replying to lancewillett:

This is what Aaron and I set out to do in #17979. We're treating the saved data as canonical, and moving anything else to Orphaned widgets.

So the patch is correct in that sense.

Then we should expire that data at some point. If a week is too short, perhaps a month?

Replying to azaozz:

In that case IMHO we should be adding all of the newly added widgets to Inactive Widgets.

Please, no. :)

We want to avoid moving anything to Inactive if possible. Remap to new sidebar is 1st priority, which works in all cases but this one.

Second priority is to move to Orphaned.

My goal is to not ever see anything in Inactive unless a user moves it there.

Unfortunately this ends up happening with 19092.2.patch too. New widgets instances end up in Inactive as we treat the old saved data as main/canonical.

All widgets that were added after the themes have been switched end up in Inactive. In fact if we expire the saved data (after some time) the currently used widgets are matched better to the old theme's sidebars.

Not sure what would be the best thing to do. Looking at individual widget instances is not the answer, perhaps we could compare the current data with the saved data and append any newly added widgets to the sidebars that match the positions? I.e. when looking for extra widgets instances we honor their position in the previous theme although they aren't in the saved data.

So, new data:

  • sidebar A has 7 widgets, 3 old and 4 new,
  • sidebar B has 3 widgets, all old,
  • sidebar C has 1 widget, new.

Saved data:

  • sidebar A has 2 widgets that match 2 of the 3 above,
  • sidebar B has 3 widgets that match the 3 above,
  • sidebar C has 3 widgets, all missing,
  • sidebar D has 2 widgets, 1 in sidebar A above, 1 in Inactive,
  • sidebar E has 1 widget, missing.

After restoring the saved data we end up with:

  • sidebar A has 2 widgets out of the 7 that were there in the previous theme,
  • sidebar B has 3 widgets that match the 3 previous,
  • sidebar C has 0 widgets,
  • sidebar D has 2 widgets, 1 from the previous sidebar A,
  • sidebar E has 0 widgets,
  • Inactive Widgets have 4 widgets that weren't in the saved data.

Note that both new data and saved data have a total of 11 widgets but we end up moving 4 to Inactive when restoring the saved data.

(wow, that's a long comment...)

Last edited 13 years ago by azaozz (previous) (diff)

#8 in reply to: ↑ 7 ; follow-up: @aaroncampbell
13 years ago

Replying to azaozz:

Replying to lancewillett:

This is what Aaron and I set out to do in #17979. We're treating the saved data as canonical, and moving anything else to Orphaned widgets.

So the patch is correct in that sense.

Then we should expire that data at some point. If a week is too short, perhaps a month?

Just thought I'd chime in that Lance and I thought the same thing (You can probably see this by reading #17979). However, this was decided in one of the dev meetings and we were overruled.

#9 in reply to: ↑ 2 ; follow-ups: @azaozz
13 years ago

Replying to lancewillett:

It seems 19291.patch makes new orphaned sidebar for each orphaned widget. Was that intended?

Last edited 13 years ago by azaozz (previous) (diff)

#10 in reply to: ↑ 9 @azaozz
13 years ago

(clicked Reply instead of Edit)

Last edited 13 years ago by azaozz (previous) (diff)

#11 in reply to: ↑ 9 ; follow-up: @lancewillett
13 years ago

Replying to azaozz:

Replying to lancewillett:

It seems 19291.patch makes new orphaned sidebar for each orphaned widget. Was that intended?

Yes, since once you get to the "lost widget" check the concept of the related sidebar is lost. I'd love to group them, but wasn't sure if that was needed.

@lancewillett
13 years ago

Refreshed my patch to remove changes in r19340.

#12 in reply to: ↑ 8 @lancewillett
13 years ago

Replying to aaroncampbell:

Replying to azaozz:

Replying to lancewillett:

This is what Aaron and I set out to do in #17979. We're treating the saved data as canonical, and moving anything else to Orphaned widgets.

So the patch is correct in that sense.

Then we should expire that data at some point. If a week is too short, perhaps a month?

Just thought I'd chime in that Lance and I thought the same thing (You can probably see this by reading #17979). However, this was decided in one of the dev meetings and we were overruled.

Can we bring this up again? I think based on Andrew's feedback it doesn't make sense to persist the data longer than a month.

Replying to azaozz:

Not sure what would be the best thing to do.

This patch should take care of that by moving the non-matching widgets to the orphan area, one per widget.

(wow, that's a long comment...)

:)

#13 follow-ups: @jane
13 years ago

I'm still unclear on some of this. If a user changes their theme and doesn't mess with widgets because they were just test-driving, I can see having the widgets from pre-switch return. But if they change back a month later, have messed with widgets, etc, is it going to put them back to where they were a month ago? This feature needs to be well-documented. Can someone who worked on it write up a description of how this is meant to work and what the parameters are so me can use it in the help screens and put it in the Codex?

#14 in reply to: ↑ 13 @SergeyBiryukov
13 years ago

Replying to jane:

Can someone who worked on it write up a description of how this is meant to work and what the parameters are so me can use it in the help screens and put it in the Codex?

Related: ticket:19020:30.

#15 in reply to: ↑ 13 ; follow-up: @azaozz
13 years ago

Replying to jane:

If a user changes their theme and doesn't mess with widgets because they were just test-driving, I can see having the widgets from pre-switch return.

Yes, that's how it works at the moment.

But if they change back a month later, have messed with widgets, etc, is it going to put them back to where they were a month ago?

Reverting to the previous theme will put back the widgets that existed when the theme was last active (data was saved). Doesn't matter if the individual widgets settings were changed or not. However new instances of the widgets, i.e. widgets that were added by dragging them to a sidebar, will currently end up in Inactive.

This feature needs to be well-documented. Can someone who worked on it write up a description of how this is meant to work and what the parameters are so me can use it in the help screens and put it in the Codex?

I can try, although not that good at it :)


Each theme defines different number of sidebars or widget areas that show on the right side of the screen. After switching to a new theme the new sidebars are displayed on the right and if the old theme had greater number of sidebars, they are displayed under the Available Widgets area still containing the widgets that were added to them.

These sidebars and widgets do not appear on the site. If you want to use any of the widgets there, please drag them to a sidebar on the right.

Another feature is the backup of sidebars configurations for different themes. In the above example, if you switch back to your old theme all widgets that still exist (weren't deleted) will be moved back to their previous places. Widgets that were added after you switched themes will be moved to the Inactive Widgets area, ready to be added to a sidebar. This makes it a breeze to try new themes without loosing your existing configuration.


#16 in reply to: ↑ 11 @azaozz
13 years ago

Replying to lancewillett:

Yes, since once you get to the "lost widget" check the concept of the related sidebar is lost. I'd love to group them, but wasn't sure if that was needed.

This can look confusing... Imagine 10 orphaned widgets that will create 10 additional orphaned sidebars under Available Widgets, each containing just one widget.

I understand why you don't want to add them to Inactive, not to mix them with the (many) widgets that are probably already there, but having many orphaned sidebars containing one widget each may be worse.

#17 in reply to: ↑ 15 ; follow-up: @jane
13 years ago

Replying to azaozz:

Reverting to the previous theme will put back the widgets that existed when the theme was last active (data was saved). Doesn't matter if the individual widgets settings were changed or not. However new instances of the widgets, i.e. widgets that were added by dragging them to a sidebar, will currently end up in Inactive.

This seems like the exact opposite of the original intent (don't lose widgets when changing themes). If I used a new theme for two weeks and then changed back b/c I didn't like it, losing (to Inactive) all the widgets I've added/messed with in the past two weeks would really freak me out. I'm officially registering (again) my discomfort with how this feature was added, for the record.

#18 in reply to: ↑ 17 ; follow-ups: @azaozz
13 years ago

Replying to jane:

This seems like the exact opposite of the original intent (don't lose widgets when changing themes). If I used a new theme for two weeks and then changed back b/c I didn't like it, losing (to Inactive) all the widgets I've added/messed with in the past two weeks would really freak me out.

In practice it works better: after switching themes to a brand new one (no saved data), widgets are matched sidebar by sidebar, so most if not all widgets from the old theme end up in the new theme.

Then when switching back to the old theme, the positions of the widgets are restored to what they were originally. Only if new widgets were added after switching themes they go into inactive. If old widgets were edited they still go to their previous places.

So switching to a new theme, editing the widgets a bit then switching back will restore them 100% (and keep any individual widget settings no matter when it was edited).

The comments above are mostly about how to handle the restoration of widgets on switching back.

#19 in reply to: ↑ 18 @jane
13 years ago

Replying to azaozz:

Only if new widgets were added after switching themes they go into inactive. If old widgets were edited they still go to their previous places.

Exactly. You lose widgets.

#20 in reply to: ↑ 18 ; follow-up: @DrewAPicture
13 years ago

Replying to azaozz:

Then when switching back to the old theme, the positions of the widgets are restored to what they were originally. Only if new widgets were added after switching themes they go into inactive. If old widgets were edited they still go to their previous places.

So switching to a new theme, editing the widgets a bit then switching back will restore them 100% (and keep any individual widget settings no matter when it was edited).

Since the wording here is obviously still a bit dense, maybe you can clarify something. I get that previously assigned widgets will return if you revert back to the original theme, but what happens if you change the saved widget data? Does the data revert to what it was before or just the widget being assigned to X sidebar?

Replying to jane:

Replying to azaozz:

Only if new widgets were added after switching themes they go into inactive. If old widgets were edited they still go to their previous places.

Exactly. You lose widgets.

I'm not sure that actually means you're losing widgets though. Sure, they're getting dumped into "Inactive Widgets" but I thought the reasoning behind "Inactive" was to save previously configured widgets that weren't in use. Thus, you don't "lose" anything, you just may have to reassign the widgets to sidebars.

Last edited 13 years ago by DrewAPicture (previous) (diff)

#21 @azaozz
13 years ago

Replying to jane:

Agree, that's not the best behavior. Added a suggestion about this in an earlier comment:
"...perhaps we could compare the current data with the saved data and append any newly added widgets to the sidebars that match the positions..."

#22 in reply to: ↑ 20 @azaozz
13 years ago

Replying to DrewAPicture:

...what happens if you change the saved widget data? Does the data revert to what it was before or just the widget being assigned to X sidebar?

Just the widget position in a sidebar. The widget's settings are not touched through all this.

Last edited 13 years ago by azaozz (previous) (diff)

#23 @azaozz
13 years ago

And another suggestion: currently after activating new theme we return the user to Appearance->Themes. There's nothing the user can do on that screen any more and we show a notice to check the widgets screen.

Wouldn't it be better to redirect them to Appearance->Widgets instead (if the new theme supports widgets of course). That would serve two purposes: letting them fix/rearrange widgets that may not have been put in the right place and run our code in wp-admin/widgets.php that matches sidebars between the old and the new theme.

Version 0, edited 13 years ago by azaozz (next)

#24 follow-up: @PaulGregory
13 years ago

DrewAPicture said:

I thought the reasoning behind "Inactive" was to save previously configured widgets that weren't in use. Thus, you don't "lose" anything, you just may have to reassign the widgets to sidebars.

Me too. But not lancewillet, who filed bug #17979 "Avoid losing widgets when switching themes", which is how we got into this mess. It's from there that we get the usage of "lose" and "lost" to refer to widgets which are/were quite easily findable in what was essentially a Lost Property box. In some cases it may be being deployed with sarcasm.

To recap that ticket:

The solution to "losing" Widgets Not Assigned To Active Sidebars into the Inactive Widgets box was to forcefully assign widgets to active sidebars. First come, first served. Landscape AdWords banner from "Top Banner"? In it goes to first slot "Right Banner 1". In it goes! All shall have homes. No widget left behind.

It was erroneously thought that the issues caused by moving back to the original theme would be solved by creating a backup of the widget configuration. Issues caused by that set of configuration being out of date were then 'solved' by introducing an expiry date.

So instead of just having widgets being "lost" in inactive widgets, we now able to misplace them in space and time.

#25 @jane
13 years ago

Disagreeing with someone is no reason to get snippy, @PaulGregory. It won't win you or your opinions any respect, especially without a history of constructive contribution to fall back on. If you want to make a suggestion, contribute a patch, or point out a bug, please do so, but do it respectfully. Thanks.

#26 in reply to: ↑ 24 @DrewAPicture
13 years ago

Replying to PaulGregory:

Me too. But not lancewillet, who filed bug #17979 "Avoid losing widgets when switching themes", which is how we got into this mess.

It's my understanding that the terminology being used is the problem. I think lancewillet was specifically referring to widget's sidebar 'assignments' being lost and not actually losing widgets to the "inactive" box.

I think this concept may just need to be clearly spelled out for users of 3.3 then revisted in 3.4 as jane previously suggested.

#27 @azaozz
13 years ago

  • Keywords 3.4-early added
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

Thinking that what Jane reported was caused by tweaking that code so many times in trunk. Like @lancewillett I could only reproduce something similar after removing all widgets from all sidebars. However the suggestions in this ticket are an enhancement to what we currently have.

#28 @nacin
11 years ago

  • Component changed from Warnings/Notices to Widgets

#29 @chriscct7
9 years ago

  • Keywords needs-patch needs-testing added; 3.4-early removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed

Closing as maybelater. Complete lack of interest on the issue on the ticket over the last 4 years. Feel free to reopen when more interest re-emerges (particularly if there's a patch).

Note: See TracTickets for help on using tickets.