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

Adds standard text editor FIND functionality to source editor #3551

Closed
wants to merge 8 commits into from

Conversation

dicene
Copy link
Contributor

@dicene dicene commented Mar 31, 2020

Brief overview of PR changes/additions

Adds a small Find menu with the shortcut CTRL+F that allows you to search in an individual document quickly, rather than searching the entire profile.

Motivation for adding to Mudlet

Document searching is ubiquitous in code editors. Even though the profile search feature CAN handle searching in the same document, it's not as convenient as being able to CTRL+F then type a search and press enter to immediately hop to the first result, then pressing ENTER or SHIFT+ENTER to go forward and backwards through results. Additionally, many modern IDE's highlight every instance of what you've typed into the search box, giving you an easy way to scroll a script on your own, but with instances of your search made very apparent.

Other info (issues closed, discussion etc)

image

…lows you to search in an individual document quickly, rather than searching the entire profile.
@dicene dicene requested review from a team as code owners March 31, 2020 01:30
@add-deployment-links
Copy link

add-deployment-links bot commented Mar 31, 2020

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@dicene
Copy link
Contributor Author

dicene commented Mar 31, 2020

Note: There are some comments left in the code that I will remove in a commit, but I wanted to get a functional version up quickly for feedback. Additionally, it's a little hard to see the pre-search outlines with that color-scheme. Here's another screenshot with the Mudlet color scheme. I know it's not accounting for the horizontal scrollbar, I'll fix that in a commit right now.

@Edru2
Copy link
Member

Edru2 commented Mar 31, 2020

Thank you for this! This is very nice. Wanted this for a long time.
👍

I see no way to let the small finder menu disappear,
but I'm sure the functionality is just not added yet.

@vadi2
Copy link
Member

vadi2 commented Mar 31, 2020

Seconded! New files to need to be added to cmake as well, that's why some builds aren't yet passing.

@dicene
Copy link
Contributor Author

dicene commented Mar 31, 2020

Thank you for this! This is very nice. Wanted this for a long time.
👍

I see no way to let the small finder menu disappear,
but I'm sure the functionality is just not added yet.

You can manually get rid of it via ESC, with is the standard way of closing those sorts of things in IDE's that I use. Probably a good idea to add a manual close button as well, I'll get on that!

@SlySven
Copy link
Member

SlySven commented Mar 31, 2020

Haven't looked at this one yet, but off the top of my head I thought <F3>/<Shift>+<F3> were the keys for find next / find previous...

@@ -8133,6 +8224,11 @@ bool dlgTriggerEditor::event(QEvent* event)
return QMainWindow::event(event);
}

void dlgTriggerEditor::resizeEvent(QResizeEvent* event){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line-feed before the { please...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, thought I had caught all the new areas and Beautified. Missed this one!

<height>0</height>
</size>
</property>
<property name="inputMask">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This, and the next two properties are default aren't they? If so, it is probably safer to trim them out of the .ui file (by hand) to keep them in the defaulted state - (IIRC there is a little reverse icon in the Qt Designer plug-in/ stand-alone utility which might do that (and turn the setting back to the dim default setting) rather than leaving it over-ridden (and thus in bold) but set to the default in that tool...

<property name="autoFillBackground">
<bool>true</bool>
</property>
<property name="styleSheet">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful - do we really need a style-sheet set on this - as it will interfere with the end-user (or the Desktop Environment) doing so. If it isn't needed I would recommend taking this property out with a bit of editing this file with the plain text editor...

<verstretch>0</verstretch>
</sizepolicy>
</property>
<property name="text">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might consider using a symbol (how about the green double triangles ones we use for the searching on the main TConsole) rather than text for this and the next button (though then they will need something for the Qt Accessibility work that is starting up) so that a translation is not needed for normally able users.

@SlySven
Copy link
Member

SlySven commented Apr 2, 2020

🤔 BTW What happens if the main Editor search is used at the same time as this one is active? 😜

@SlySven
Copy link
Member

SlySven commented Apr 5, 2020

Blooming 'eck - something really screwy is happening as a result of that last commit dc2e26d as we get compilation errors about the use of a (publically, so THAT is not the problem) enum defined in the dlgTriggerEditor class but which is not defined at the point it is seen in the Host.h header file - even though that same header HAS the #include "dlgTriggerEditor.h" at the top:
image

Even adding a class specifier (as I had done in the above screen shot):

     void setPlayerRoomStyleDetails(const quint8 styleCode, const quint8 outerDiameter = 120, const quint8 innerDiameter = 70, const QColor& outerColor = QColor(), const QColor& innerColor = QColor());
     void getPlayerRoomStyleDetails(quint8& styleCode, quint8& outerDiameter, quint8& innerDiameter, QColor& outerColor, QColor& innerColor);
-    void setSearchOptions(const SearchOptions);
+    void setSearchOptions(const dlgTriggerEditor::SearchOptions);

     cTelnet mTelnet;

Doesn't fix it... 😖

@dicene
Copy link
Contributor Author

dicene commented Apr 7, 2020

I really don't enjoy trying to update branches like this to be current with the dev branch. At best, it's an annoyance, at worst things like this happen...

…eSearch"

This reverts commit dc2e26d, reversing
changes made to 2b53dcd.
@dicene dicene requested a review from a team as a code owner April 7, 2020 15:25
@vadi2
Copy link
Member

vadi2 commented Apr 7, 2020

@dicene still need to add the files you've added to cmake - if you could do that that'll fix the rest of the builds :)

@dicene
Copy link
Contributor Author

dicene commented Apr 7, 2020

Running a test build then committing cmake updates right now.

@dicene
Copy link
Contributor Author

dicene commented Apr 8, 2020

Superceded by #3578

@dicene dicene closed this Apr 8, 2020
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

4 participants