-
-
Notifications
You must be signed in to change notification settings - Fork 220
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
Add a command to navigate to a certain page by incremental search #1044
base: main
Are you sure you want to change the base?
Conversation
using var one = new OneNote(); | ||
//var service = new SearchServices(owner, one, sectionId); | ||
|
||
if (copying) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these operations going to be implemented? If not, remove the code and associated UI controls from the dialog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this file was touched? If not, please revert the changes.
public bool FallThrough { get; set; } | ||
//public string NotebookXml => GetAllPages(); //onenote.Windows.CurrentWindow?.CurrentNotebookId; | ||
|
||
public XElement GetAllPages() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be moved down alphabetically amongst the methods below
{ | ||
onenote.GetHierarchy(null, HierarchyScope.hsPages, out string xml, XMLSchema.xs2013); | ||
if (!string.IsNullOrEmpty(xml)) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a lot of value in this helper function beyond what GetHierarchy already provides. I'd suggest just inlining it within your business logic rather than adding to the OneNote class.
/// <returns></returns> | ||
public async Task MovedAfterPage(string CurPageId, string DestPageId) | ||
{ | ||
var root = GetAllPages(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very inefficient. There are easier ways of discovering, tracking, and using the notebook and section IDs along with the page and target IDs to make this faster.
@@ -3446,6 +3446,10 @@ ISO-code then comma then language name</comment> | |||
<value>بحث ونسخ / نقل</value> | |||
<comment>ribbon item</comment> | |||
</data> | |||
<data name="ribNaviButton_Label" xml:space="preserve"> | |||
<value>FastNavi</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better name than "FastNavi"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix it. How about "Incremental Search"?
FilteredPageXml = FilterXml(AllPageXML, strKeyWords); | ||
} | ||
|
||
hierarchyView11.Nodes.Clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a poor use of TreeView, causing flickering and slowness with each keystroke. You also don't need to fetch AllPages on every keystroke since it is unlikely the hierarchy would change while this dialog is open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a poor use of TreeView, causing flickering and slowness with each keystroke. You also don't need to fetch AllPages on every keystroke since it is unlikely the hierarchy would change while this dialog is open.
It seems that this variable AllPageXML is passed by reference, not by value.
If the value is not fetched very loop, its value will be changed by the function FilterXML.
I've tried to define a class with a copy element, but still no luck here.
As for the flickering, I trid serveral methods, also no luck.
Could you give any comments on how to solve those two problems? Thanks.
Honestly, I'm trying to understand where this really belongs. It's functionally equivalent to Search/SearchDialog but with type-ahead, which I'm not sure is enough of a reason for a new dialog. What do you see as the primary difference? |
|
@valuex your streaks ahead of me on it though. I like incremental searches. I didn't realise the power of the exisitng Search & copy Move. I would put your tool in "navigate" is navigate became tabbed with hotkeys for move between them. I would probably put Fav / Nav / Incremental all on a dockable form and be able to turn tabs on / off with preferences. ... But at the moment I would put it on its own "experimental" tabbed form, then we can have stuff that isn't completely baked in and not spend too long on consistency while we work out if it is gold or "may be a good idea". |
IS YOUR COMMIT SIGNED?
Note that this repo requires all commits to be properly signed, see here for more info
Yes
Enhancement or Defect Addressed by This PR
Description of Proposed Changes