Search: Add breadcrumbs to search results in order to provide more context#2282
Open
Tschuppi81 wants to merge 44 commits intomasterfrom
Open
Search: Add breadcrumbs to search results in order to provide more context#2282Tschuppi81 wants to merge 44 commits intomasterfrom
Tschuppi81 wants to merge 44 commits intomasterfrom
Conversation
❌ 32 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
Author
|
Also i switched the red color in the results to primary and italic left: previous, right: now |
Contributor
Author
|
Not sure if the breadcrumbs color in search shall be |
There was a problem hiding this comment.
Pull request overview
This pull request adds breadcrumb navigation to search results for various content types (Topics, News, People, Files, Tickets, Events, Directory Entries, etc.) to provide better context when viewing search results. The implementation includes:
Changes:
- Renamed
PageLayouttoTopicLayoutthroughout the codebase for clarity - Added a new
get_layout()method to the request object to retrieve layouts for model instances - Implemented a layout registry system via a new
Layoutdirective - Updated search result templates to display breadcrumbs for various content types
- Added breadcrumb styling and fixed CSS issues related to z-index stacking
- Added file ID anchors and highlight styling for targeted files
- Updated test selectors to use regex anchors for more precise matching
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/onegov/winterthur/test_search.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/town6/test_views_directory.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/org/test_views_ticket.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/org/test_views_directory.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/org/test_layout.py | Updated imports from PageLayout to TopicLayout |
| src/onegov/town6/theme/styles/town6.scss | Added z-index reset for breadcrumb links |
| src/onegov/town6/theme/styles/search.scss | Added breadcrumb styling and updated search preview styles |
| src/onegov/town6/theme/styles/files.scss | Added highlight and scroll margin for targeted file rows |
| src/onegov/town6/templates/macros.pt | Added breadcrumbs to search result macros, changed p to div tags for consistency |
| src/onegov/town6/layout.py | Renamed PageLayout to TopicLayout, added layout decorators for multiple models, implemented breadcrumbs for GeneralFile and various RIS models |
| src/onegov/org/views/page.py | Updated imports and references from PageLayout to TopicLayout |
| src/onegov/org/views/editor.py | Updated imports and references from PageLayout to TopicLayout |
| src/onegov/org/request.py | Added get_layout() method to retrieve layouts for model instances |
| src/onegov/org/layout.py | Renamed PageLayout to TopicLayout, added FormDefinitionLayout, DirectoryLayout, improved breadcrumbs for various layouts |
| src/onegov/org/exports/base.py | Fixed import to use onegov.org instead of onegov.town6 |
| src/onegov/org/directives.py | Added Layout directive for registering layouts to models |
| src/onegov/org/app.py | Added layout directive to OrgApp |
| src/onegov/agency/views/page.py | Updated imports and references from PageLayout to TopicLayout |
| src/onegov/agency/layout.py | Updated imports and class name from PageLayout to TopicLayout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/onegov/town6/layout.py
Outdated
Comment on lines
1238
to
1242
| Link( | ||
| self.model.meeting.title, | ||
| self.request.link(self.model.meeting) | ||
| ), | ||
| Link(title, self.request.link(self.model)) |
There was a problem hiding this comment.
The breadcrumbs list includes both self.model.meeting.title (line 1239) and title (line 1242), where title is the same meeting title with an optional date suffix. This creates redundant breadcrumb entries pointing to the same meeting but with different labels. Consider removing the duplicate entry or clarifying the intended breadcrumb structure.
src/onegov/org/directives.py
Outdated
Comment on lines
313
to
320
| layout_registry: dict[type, Layout] | ||
| ) -> str: | ||
| return str(self.model) | ||
|
|
||
| def perform( # type:ignore[override] | ||
| self, | ||
| layout: Layout, | ||
| layout_registry: dict[type, Layout] |
Copilot
AI
Jan 13, 2026
•
There was a problem hiding this comment.
The type annotation for layout_registry should be dict[type, type] instead of dict[type, Layout] since the layout parameter represents a layout class (type), not an instance of the Layout directive class. The perform method stores layout classes in the registry, not Layout directive instances.
Suggested change
| layout_registry: dict[type, Layout] | |
| ) -> str: | |
| return str(self.model) | |
| def perform( # type:ignore[override] | |
| self, | |
| layout: Layout, | |
| layout_registry: dict[type, Layout] | |
| layout_registry: dict[type, type[Layout]] | |
| ) -> str: | |
| return str(self.model) | |
| def perform( # type:ignore[override] | |
| self, | |
| layout: type[Layout], | |
| layout_registry: dict[type, type[Layout]] |
Member
There was a problem hiding this comment.
I cleaned up the type annotations to still include Layout, but otherwise a correct suggestion.
Comment on lines
150
to
155
| .search-preview em { | ||
| color: $primary-color !important; | ||
| font-style: italic; | ||
| font-weight: bold; | ||
| text-decoration: underline; | ||
| } |
There was a problem hiding this comment.
The CSS selector has been changed from .search-results em to .search-preview em, narrowing the scope to only emphasize text within search preview sections. This change may affect emphasis styling for other parts of search results that fall outside the search-preview div.
BreathingFlesh
requested changes
Jan 13, 2026
Contributor
BreathingFlesh
left a comment
BreathingFlesh
left a comment
There was a problem hiding this comment.
I would give the breadcrumbs a light background so they look more like a separate element
Contributor
Author
|
Here two examples of how search results look 
|
…m:OneGov/onegov-cloud into feature/ogc-2880-search-results-with-path
Daverball
reviewed
Jan 14, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks good, but we should also register layouts for Org. Other than there's some minor things.
We can probably do a better job for GeneralFile, since only publications will have a simple link to a list of files, we should look at which models have linked to the file and then display the breadcrumbs for each of those models. Performance is definitely a concern when looking for links, so we should probably cache them on the model, just like we cache the access of the linked models. But we can improve handling of general files in a follow-up change, for now I would only display breadcrumbs for publications, we can maybe link to the files management view for logged in users, but I'm not sure it's worth it.
src/onegov/org/directives.py
Outdated
Comment on lines
313
to
320
| layout_registry: dict[type, Layout] | ||
| ) -> str: | ||
| return str(self.model) | ||
|
|
||
| def perform( # type:ignore[override] | ||
| self, | ||
| layout: Layout, | ||
| layout_registry: dict[type, Layout] |
Member
There was a problem hiding this comment.
I cleaned up the type annotations to still include Layout, but otherwise a correct suggestion.
Daverball
reviewed
Jan 14, 2026
Comment on lines
288
to
304
| def get_layout(self, model: object) -> Layout | DefaultLayout: | ||
| """ | ||
| Get the registered layout for a model instance. | ||
| """ | ||
| layout_registry = self.app.config.layout_registry | ||
| model_type = model if isinstance(model, type) else type(model) | ||
|
|
||
| layout_class = None | ||
| for cls in model_type.mro(): | ||
| layout_class = layout_registry.get(cls) | ||
| if layout_class: | ||
| break | ||
|
|
||
| if layout_class is None: | ||
| layout_class = DefaultLayout | ||
|
|
||
| return layout_class(model, self) |
Member
There was a problem hiding this comment.
You could probably simplify this by instead relying on a dispatch_method on Framework, which could look something like this:
@dispatch_method()
def get_layout_class(self, model: object) -> type[Layout] | None:
return None
...
@Framework.predicate(Framework.get_layout_class, name="model", default=None, index=ClassIndex)
def model_predicate(self, model: object) -> type:
return model if isinstance(model, type) else model.__class__which simplifies get_layout to
def get_layout(self, model: object) -> Layout:
"""
Get the registered layout for a model instance.
"""
layout_class = self.app.get_layout_class(model)
if layout_class is None:
layout_class = DefaultLayout
return layout_class(model, self)You could also register a predicate_fallback, so get_layout_class returns DefaultLayout if the predicate doesn't match:
@OrgApp.predicate_fallback(OrgApp.get_view, model_predicate)
def model_not_found(self, model: object) -> type[Layout]:
return DefaultLayout
which simplifies get_layout to
def get_layout(self, model: object) -> Layout:
"""
Get the registered layout for a model instance.
"""
layout_class = self.app.get_layout_class(model)
assert layout_class is not None
return layout_class(model, self)and lets you override the fallback in TownApp with its own DefaultLayout.
You then also no longer need a registry for the LayoutAction, you instead register with the dispatch method:
def perform(self, layout: type[Layout], app_class: type[Framework]) -> None:
app_class.get_layout_class.register(layout, model=self.model)
Contributor
Author
There was a problem hiding this comment.
I tried various things but didn't manage to register properly. Always got the DefaultLayout.
(Even Claude and Gemini got very confused about morepath and reg)
trial branch: #2302
Member
There was a problem hiding this comment.
think I got confused about predicates with view, since get_view has an obj argument, and a model predicate, they're different and probably need to be different
So I think you need to change model to obj on get_layout and then also rename the parameter in model_predicate and model_not_found. Basically you can copy this from get_view without the extra request parameter.
You also need to update your directives perform method, so it calls the register method on the dispatch_method. I see you created a custom decorator for testing, but it's possible that that won't work, since there's some very specific magic about how perform on actions is invoked with applications that inherit from one another. This may break if you just register the dispatch immediately with a custom decorator.
You can also test the predicate by checking what app.get_layout.by_predicates(model=MyModel) returns.
Contributor
Author
There was a problem hiding this comment.
Oh, I will still need my directive? I though I will replace the directive by the predicate...
Member
There was a problem hiding this comment.
It replaces the registry the directive uses with a dispatch_method not the directive itself. The builtin view directive from morepath also works this way, it doesn't have a registry like the other actions but uses the get_view dispatch_method to register the views.
Member
There was a problem hiding this comment.
Member
There was a problem hiding this comment.
We won't need a wrapper object, like they created for views however, since we only want the Layout class itself, we don't care about additional metadata, so you can directly call get_layout.register with the Layout class and the model= predicate, we don't need to support arbitrary predicates.
Contributor
Author
|
Thank your for your feedback @BreathingFlesh regarding styling. Latest look 
|
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Tschuppi81
changed the title
Contributor
Author
|
Just pushed a change, still no luck to get the layout registered. Same with the fallback registration. |
Daverball
reviewed
Jan 30, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
There's some confusion about the use of app_class in an Action class, I wonder if that's the reason why it is still not working. I feel like this should be working, since views aren't doing anything more special than we're doing with layouts. It also seems slightly better suited for looking up layouts to me, than a plain dictionary.
src/onegov/org/directives.py
Outdated
Comment on lines
326
to
329
| app_class.get_layout_class.register( | ||
| model=self.model, | ||
| func=lambda app_class, model: obj | ||
| ) |
Member
There was a problem hiding this comment.
This does not seem right, I believe this should be simply the following:
Suggested change
| app_class.get_layout_class.register( | |
| model=self.model, | |
| func=lambda app_class, model: obj | |
| ) | |
| app_class.get_layout_class.register(obj, model=self.model) |
Contributor
Author
There was a problem hiding this comment.
Getting a signature issue with app_class.get_layout_class.register(obj, model=self.model)
reg.error.RegistrationError: Signature of callable dispatched to (<class 'onegov.town6.layout.PageLayout'>) not that of dispatch (<function Framework.get_layout_class at 0x7b2f14768e00>)
Member
There was a problem hiding this comment.
Oh, you're right I didn't notice that View had a __call__ method that matched get_view, you can still pass the argument positionally though, instead of by name. You should also change app_class to self in both the lambda and the dispatch method. It is a method after all, so the first argument is self, the same goes for the predicate and its fallback, where the first argument is also self, in all instances it's an instance of the current application, i.e. Framework, although you won't need to annotate the type on get_layout_class only the predicate/fallback.
Member
There was a problem hiding this comment.
It might however be worth considering switching from get_layout_class to get_layout and add a request parameter, then Layout.__init__ matches the parameters of get_layout and you get a layout instance for the given model instance and request. So you can directly pass in Layout instead of having to wrap it in a lambda.
It might also be worth considering adding a name predicate so you can overwrite the layout for specific views. That would mean we would no longer have to overwrite Org views in Town6 just to pass the Town6 version of the layout, we just always retrieve the layout using get_layout in the view which will retrieve us the application/view specific layout. But that's a cleanup for later. For now we should just consider prioritizing get_layout over get_layout_class so we have a better API for that.
Contributor
Author
|
Ah! I needed to the tell the Action class to inject |
Tschuppi81
commented
Feb 2, 2026
src/onegov/core/directives.py
Outdated
| app_class.get_layout.register( # type:ignore[attr-defined] | ||
| model=self.model, | ||
| request_class=self.request, | ||
| func=layout_for_obj |
Contributor
Author
There was a problem hiding this comment.
I tried to pass a layout instance func=obj(self.model, self.request) but it requires a request instance which I don't have, onyl the request class..
@Daverball That was, what you mentioned, right?
Member
There was a problem hiding this comment.
func is a replacement for the get_layout function for the given predicate value of model, so it needs to accept the same parameters as get_layout, the idea is to pass obj as func, since Layout.__init__ has the same parameters as get_layout. So you don't call obj here, it only gets called when you call get_layout, with the same parameters you passed into get_layout.
A dispatch_method performs dynamic dispatch using the registered predicates, so it first calls all the predicates to see which version of get_layout needs to be invoked for the given parameters (internally this happens through get_layout.for_predicates(model=obj.__type__) which should then return a Layout class for that model, if it has one or the predicate fallback if it doesn't, subsequently this gets called with the same parameters as get_layout).
Daverball
reviewed
Feb 2, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
I think there was some confusion, request is not a predicate it's just a regular parameter on get_layout, so you get back a layout instance for the given obj and request, you don't get a layout class back anymore.
src/onegov/core/directives.py
Outdated
Comment on lines
402
to
429
| def __init__(self, model: type, request: CoreRequest) -> None: | ||
| self.model = model | ||
| self.request = request | ||
|
|
||
| def identifier( # type:ignore[override] | ||
| self, | ||
| app_class: type[Framework] | ||
| ) -> str: | ||
| return str(self.model) | ||
|
|
||
| def perform( # type:ignore[override] | ||
| self, | ||
| obj: type[Layout], | ||
| app_class: type[Framework] | ||
| ) -> None: | ||
|
|
||
| def layout_for_obj( | ||
| app_self: type[Framework], | ||
| model: object, | ||
| request: CoreRequest | ||
| ) -> type[Layout]: | ||
| return obj | ||
|
|
||
| app_class.get_layout.register( # type:ignore[attr-defined] | ||
| model=self.model, | ||
| request_class=self.request, | ||
| func=layout_for_obj | ||
| ) |
Member
There was a problem hiding this comment.
request is a not a predicate, it's just an additional parameter that gets passed into get_layout, since you need it to create an instance of Layout, so the idea here is to simplify this to the following:
since Layout(obj, request) invokes Layout.__init__(obj, request) which will return a Layout instance for the passed in object and request.
Suggested change
| def __init__(self, model: type, request: CoreRequest) -> None: | |
| self.model = model | |
| self.request = request | |
| def identifier( # type:ignore[override] | |
| self, | |
| app_class: type[Framework] | |
| ) -> str: | |
| return str(self.model) | |
| def perform( # type:ignore[override] | |
| self, | |
| obj: type[Layout], | |
| app_class: type[Framework] | |
| ) -> None: | |
| def layout_for_obj( | |
| app_self: type[Framework], | |
| model: object, | |
| request: CoreRequest | |
| ) -> type[Layout]: | |
| return obj | |
| app_class.get_layout.register( # type:ignore[attr-defined] | |
| model=self.model, | |
| request_class=self.request, | |
| func=layout_for_obj | |
| ) | |
| def __init__(self, model: type) -> None: | |
| self.model = model | |
| def identifier( # type:ignore[override] | |
| self, | |
| app_class: type[Framework] | |
| ) -> str: | |
| return str(self.model) | |
| def perform( # type:ignore[override] | |
| self, | |
| obj: type[Layout], | |
| app_class: type[Framework] | |
| ) -> None: | |
| app_class.get_layout.register(obj, model=self.model) |
Comment on lines
+81
to
+87
| @TownApp.predicate_fallback(TownApp.get_layout, layout_predicate) | ||
| def layout_not_found(self: type[TownApp], obj: object) -> type[Layout]: | ||
| # circular import | ||
| from onegov.town6.layout import DefaultLayout | ||
| return DefaultLayout | ||
|
|
||
|
|
Member
There was a problem hiding this comment.
With the changed semantics you can decorate the original definition of DefaultLayout with @TownApp.predicate_fallback(TownApp.get_layout, layout_predicate), just like you directly decorate the other layouts for their respective model. This also gets rid of the circular import.
Member
There was a problem hiding this comment.
You will also need to do the same for OrgApp and its own DefaultLayout.
Member
There was a problem hiding this comment.
Looks like it's not going to be as simple after all. We could probably do something with a custom __call__ on a metaclass that discards the first argument if it is an instance of Framework, so we could directly use Layout classes as valid implementations, without having to change the default signature for __init__. But it's probably a bit too magical, may not work with the kind of introspection morepath uses and not worth the additional effort, to make introspection work, I'd still move the registration of the fallback to where the layouts are defined, so you don't need a circular import.
It would then look something like:
@TownApp.predicate_fallback(TownApp.get_layout, layout_predicate)
def layout_not_found(self: TownApp, obj: object, request: TownRequest) -> Layout:
return DefaultLayout(obj, request)
Contributor
Author
There was a problem hiding this comment.
Yep, will move it to layouts
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Daverball
reviewed
Feb 3, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks like I forgot, that the first parameter to the dispatch_method for the app instance is required for all implementations, we still want the request parameter though, just not as a predicate and we want to return a layout instance, not a class.
Comment on lines
+1561
to
+1579
| @dispatch_method() | ||
| def get_layout( | ||
| self, | ||
| obj: object, | ||
| ) -> type[Layout] | None: | ||
| return None | ||
|
|
||
|
|
||
| @Framework.predicate( | ||
| Framework.get_layout, | ||
| name='model', | ||
| default=None, | ||
| index=ClassIndex | ||
| ) | ||
| def layout_predicate( | ||
| self: type[Framework], | ||
| obj: object, | ||
| ) -> type[Layout]: | ||
| return obj.__class__ |
Member
There was a problem hiding this comment.
Suggested change
| @dispatch_method() | |
| def get_layout( | |
| self, | |
| obj: object, | |
| ) -> type[Layout] | None: | |
| return None | |
| @Framework.predicate( | |
| Framework.get_layout, | |
| name='model', | |
| default=None, | |
| index=ClassIndex | |
| ) | |
| def layout_predicate( | |
| self: type[Framework], | |
| obj: object, | |
| ) -> type[Layout]: | |
| return obj.__class__ | |
| @dispatch_method() | |
| def get_layout( | |
| self, | |
| obj: object, | |
| request: CoreRequest | |
| ) -> type[Layout] | None: | |
| return None | |
| @Framework.predicate( | |
| Framework.get_layout, | |
| name='model', | |
| default=None, | |
| index=ClassIndex | |
| ) | |
| def layout_predicate( | |
| self: type[Framework], | |
| obj: object, | |
| request: CoreRequest | |
| ) -> type[Layout]: | |
| return obj.__class__ |
Comment on lines
+411
to
+421
| def perform( # type:ignore[override] | ||
| self, | ||
| obj: type[Layout], | ||
| app_class: type[Framework] | ||
| ) -> None: | ||
|
|
||
| layout_obj = obj | ||
| # `lambda self, obj` is required to match the signature | ||
| app_class.get_layout.register( | ||
| lambda self, obj: layout_obj, | ||
| model=self.model) |
Member
There was a problem hiding this comment.
Suggested change
| def perform( # type:ignore[override] | |
| self, | |
| obj: type[Layout], | |
| app_class: type[Framework] | |
| ) -> None: | |
| layout_obj = obj | |
| # `lambda self, obj` is required to match the signature | |
| app_class.get_layout.register( | |
| lambda self, obj: layout_obj, | |
| model=self.model) | |
| def perform( # type:ignore[override] | |
| self, | |
| obj: type[Layout], | |
| app_class: type[Framework] | |
| ) -> None: | |
| layout_class = obj | |
| # `lambda self, obj` is required to match the signature | |
| app_class.get_layout.register( | |
| lambda app, obj, request: layout_class(obj, request), | |
| model=self.model) |
Contributor
Author
There was a problem hiding this comment.
I like the idea, but need to keep the signature and if I'd pass the TownRequest or OrgRequest class to the Layout, I only have the class not the instance...
Member
There was a problem hiding this comment.
I promise you this will work, this is exactly the same way get_view is implemented, it also takes a second request parameter.
Member
There was a problem hiding this comment.
You will need to adapt OrgRequest.get_layout and the code that uses the returned layout for the breadcrumbs, but it's an overall minor change.
Member
There was a problem hiding this comment.
Remember that predicates are different from regular parameters, this is not a predicate, so you don't need to pass it in when you register the layout, it gets passed in when you call get_layout. Only predicates need to be passed eagerly and the only predicate we have currently is model, this doesn't change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Search: Add breadcrumbs to search results in order to provide more context
TYPE: Feature
LINK: ogc-2880