Web: Fix CAN options visibility for non-CAN boards#209
Web: Fix CAN options visibility for non-CAN boards#209garvit000 wants to merge 2 commits intoArduPilot:mainfrom
Conversation
| f"Redis connection established with {redis_host}:{redis_port}" | ||
| ) | ||
| self.__boards_key_prefix = "boards-" | ||
| # bump version to invalidate stale board metadata in cache |
There was a problem hiding this comment.
Please remove this change. We can handle the upgrade by manually clearing the cache.
|
|
||
| def __get_boards_at_commit_from_repo(self, remote: str, | ||
| commit_ref: str) -> tuple[list, list]: | ||
| commit_ref: str) -> tuple[list[dict], list[dict]]: |
There was a problem hiding this comment.
Needs to be documented in the comment.
| try: | ||
| non_periph_boards, periph_boards = boards | ||
| needs_upgrade = False | ||
| if non_periph_boards and isinstance(non_periph_boards[0], str): |
There was a problem hiding this comment.
Let's remove this as well. No need to worry about the upgrade due to cached stuff. We can clear it manually.
| vehicle_id=vehicle, | ||
| ) | ||
| if board not in boards_at_commit: | ||
| board_names_at_commit = [b["name"] for b in boards_at_commit] |
There was a problem hiding this comment.
Better define a class for board with id, name and attributes, in the attributed put the has_can thing.
There was a problem hiding this comment.
Defined a BoardMetadata class for above listed
|
Hey @shiv-tyagi thanks for reviewing, I have addressed all the requested changes could you re-review it. |
|
|
||
| import re | ||
|
|
||
| class BoardMetadata: |
| def __init__(self, id: str, name: str, attributes: dict): | ||
| self.id = id | ||
| self.name = name | ||
| self.attributes = attributes |
There was a problem hiding this comment.
| self.attributes = attributes | |
| self.attributes = dict(attributes) |
Let us make sure the board is not affected even it the passed attributes get mutated externally.
| "name": self.name, | ||
| "attributes": self.attributes, | ||
| } | ||
| if "has_can" in self.attributes: |
There was a problem hiding this comment.
Either keep the has_can thing at top level or inside attributes property. Not both.
| - A list contains boards for the 'ap_periph' target. | ||
| - A list of board metadata dictionaries for NON-'ap_periph' targets. | ||
| - A list of board metadata dictionaries for the 'ap_periph' target. | ||
| Each board dict exposes: id, name, attributes (has_can), and has_can (legacy). |
There was a problem hiding this comment.
I don't think there is anything like "(legacy)" in our case which we need to handle..
| def build_board_metadata(board_names: list[str]) -> list[dict]: | ||
| board_data: list[dict] = [] | ||
| for board_name in board_names: | ||
| hwdef_path = None | ||
| if hwdef_dir: | ||
| candidate_path = os.path.join(hwdef_dir, board_name, "hwdef.dat") | ||
| if os.path.isfile(candidate_path): | ||
| hwdef_path = candidate_path | ||
| else: | ||
| self.logger.debug( | ||
| "hwdef.dat not found for board %s at %s", | ||
| board_name, | ||
| candidate_path, | ||
| ) | ||
|
|
||
| has_can = self.__board_has_can(hwdef_path) if hwdef_path else False | ||
| board = BoardMetadata( | ||
| id=board_name, | ||
| name=board_name, | ||
| attributes={"has_can": has_can}, | ||
| ) | ||
| board_data.append(board.to_dict()) | ||
| return board_data | ||
|
|
||
| return ( | ||
| non_periph_boards_sorted, | ||
| periph_boards_sorted, | ||
| build_board_metadata(non_periph_boards_sorted), | ||
| build_board_metadata(periph_boards_sorted), |
There was a problem hiding this comment.
The repo needs to stay locked till you are done doing everything.
| tuple: A tuple of two lists in order: | ||
| - A list contains boards for NON-'ap_periph' targets. | ||
| - A list contains boards for the 'ap_periph' target. | ||
| - A list of board metadata dictionaries for NON-'ap_periph' targets. |
There was a problem hiding this comment.
Instead of these dictionaries. We should just return a list of Board objects. We would also need to handle the caching of those.
| list: A list of board metadata dictionaries, each containing | ||
| the board name and whether it supports CAN (has_can). |
There was a problem hiding this comment.
| list: A list of board metadata dictionaries, each containing | |
| the board name and whether it supports CAN (has_can). | |
| list: A list of Boards. |
| // Store current features for re-rendering when board changes | ||
| currentFeatures = buildOptions; | ||
|
|
||
| const selectedBoard = getSelectedBoardMetadata(); | ||
| // Default to hiding (false) if metadata is missing to be safe | ||
| const boardHasCan = selectedBoard && selectedBoard.has_can !== undefined | ||
| ? Boolean(selectedBoard.has_can) | ||
| : false; | ||
|
|
||
| let output = document.getElementById('build_options'); | ||
| output.innerHTML = `<div class="d-flex mb-3 justify-content-between"> | ||
| <div class="d-flex d-flex align-items-center"> | ||
| <p class="card-text"><strong>Available features for the current selection are:</strong></p> | ||
| </div> | ||
| <button type="button" class="btn btn-outline-primary" id="exp_col_button" onclick="toggle_all_categories();"><i class="bi bi-chevron-expand me-2"></i>Expand/Collapse all categories</button> | ||
| </div>`; | ||
|
|
||
| buildOptions.forEach((category, cat_idx) => { | ||
| if (cat_idx % 4 == 0) { | ||
| let new_row = document.createElement('div'); | ||
| new_row.setAttribute('class', 'row'); | ||
| new_row.id = 'category_'+parseInt(cat_idx/4)+'_row'; | ||
| output.appendChild(new_row); | ||
| <div class="d-flex d-flex align-items-center"> | ||
| <p class="card-text"><strong>Available features for the current selection are:</strong></p> | ||
| </div> | ||
| <button type="button" class="btn btn-outline-primary" id="exp_col_button" onclick="toggle_all_categories();"><i class="bi bi-chevron-expand me-2"></i>Expand/Collapse all categories</button> | ||
| </div>`; | ||
| let visibleCategoryIdx = 0; | ||
|
|
||
| buildOptions.forEach((category) => { | ||
| // HIDE CAN CATEGORY IF BOARD HAS NO CAN | ||
| if (category.name && category.name.includes("CAN") && boardHasCan === false) { | ||
| return; | ||
| } | ||
| let col_element = document.createElement('div'); | ||
| col_element.setAttribute('class', 'col-md-3 col-sm-6 mb-2'); | ||
| col_element.appendChild(createCategoryCard(category['name'], category['options'], init_categories_expanded)); | ||
| document.getElementById('category_'+parseInt(cat_idx/4)+'_row').appendChild(col_element); | ||
| }); | ||
| } | ||
|
|
||
| if (visibleCategoryIdx % 4 === 0) { | ||
| let new_row = document.createElement('div'); | ||
| new_row.setAttribute('class', 'row'); | ||
| new_row.id = 'category_'+parseInt(visibleCategoryIdx/4)+'_row'; | ||
| output.appendChild(new_row); | ||
| } | ||
|
|
||
| let col_element = document.createElement('div'); | ||
| col_element.setAttribute('class', 'col-md-3 col-sm-6 mb-2'); | ||
| col_element.appendChild(createCategoryCard(category['name'], category['options'], init_categories_expanded)); | ||
|
|
||
| let row = document.getElementById('category_'+parseInt(visibleCategoryIdx/4)+'_row'); | ||
| if (row) { | ||
| row.appendChild(col_element); | ||
| } | ||
|
|
||
| visibleCategoryIdx += 1; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Instead of hiding stuff at the frontend, we should make sure they don't even get returned from the backend. The filtering logic has to go in the backend.
Also, when we make some features disappear the features which are dependent on those should also disappear.
| ) | ||
| self.logger.debug(f"periph_boards sorted: {periph_boards_sorted}") | ||
|
|
||
| def build_board_metadata(board_names: list[str]) -> list[dict]: |
There was a problem hiding this comment.
Please follow the pattern in the codebase. Define a separate method in this class to do this parsing. You can prefix the method names with underscores to avoid potential external usage.
This PR fixes Issue #151 where the "CAN" feature category remained visible even when selecting a board that does not support CAN.
The Fix:
fillBuildOptionsin JS to re-render the category list whenever the board selection changes.has_can: false.includedirectives inhwdef.dat.Test Plan:
Closes #151