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

added recent search #1168

Merged
merged 4 commits into from
Aug 19, 2020
Merged

Conversation

chithihuynh
Copy link
Contributor

@chithihuynh chithihuynh commented Jul 14, 2020

QUESTIONS:

  1. How to execute: src/gui/SearchDialog.cpp (line: 742) [...if(greekText != trimmedText)...]?
  2. How to add/show my test cases for regarding my change?

Description

Order of search objects (using "F3") are now ordered from relevance recently searched then the rest of relevance searches (compare to just ordered by relevence).Recent searches are italicized so user can tell which one are recent and not.

A new search will be added to the recently searched list after the user clicks enter on an object's name within the search dialog. The list will contain the user's last 10 searches is stored in a JSON file under "/user directory/data/recentObjectSearches.json".

Fixes #1050

Screenshots (if appropriate):

Each screenshot contains the Stellarium search dialog and the corresponding /user directory/data/recentObjectSearches.json file.


Search output (no recent searches saved/how it currently is)
recentSearches01

After use clicks "enter" on "1P/Halley", then tries to search for "1P/Halley" again.
recentSearches03

User has selected "Alkalbain I" (before clicking "enter")
recentSearches04

User is searching for "l" and the following results appears.
recentSearches06

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

I think that counts as a new feature

  • Breaking change (fix or feature that would cause existing functionality to change)

It might also be this one.

  • This change requires a documentation update

Yes, if this is talking about the documentation for the new functions I put in.

How Has This Been Tested?

All test cases successfully executed locally.

I tested the new search function by searching for a few different objects and see if the expected results comes up. The expected results should include:

greekText != trimmedText (QUESTION 1: Haven't been able to get here so I couldn't test it).
greekText == trimmedText:
- 13 results total
- Most recent search first (switch are italicized) (depending on relevance)
- Other relevance searches after (similar order to original but might have fewer outputs, depends on how many recent searches matches)
- No duplicate results between the recent and other matches

Test Configuration:

  • Operating system: Ubuntu 20.04 LTS
  • Graphics Card: Manufacturer: NVidia, Model: GeForce 940MX/PCIe/SSE2, driver version: 440.100

Checklist:

  • My code follows the code style of this project.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

QUESTION 2: I wanted to add some tests but I don't know how to do so.

  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@lgtm-com
Copy link

lgtm-com bot commented Jul 14, 2020

This pull request introduces 1 alert when merging 2495773 into ed68387 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@alex-w alex-w added the feature Entirely new feature label Jul 19, 2020
@alex-w
Copy link
Member

alex-w commented Jul 19, 2020

Q1: alpha And = α And
Q2: you cannot add unit tests for Search Tool

@alex-w alex-w requested review from alex-w and gzotti July 19, 2020 21:00
@chithihuynh chithihuynh force-pushed the recentSearchOneCommit branch 2 times, most recently from 0a4a530 to c7022d3 Compare July 21, 2020 04:50
@lgtm-com
Copy link

lgtm-com bot commented Jul 21, 2020

This pull request introduces 1 alert when merging c7022d3 into 0a5ce7a - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@alex-w
Copy link
Member

alex-w commented Jul 21, 2020

Please add depth of history as option in "Options" tab and prepare binary package for windows (just use word [publish] in commit message)

@chithihuynh
Copy link
Contributor Author

I added the option to adjust the size of the list. Since Qt's max default size is 99 for spin boxes, that is where I am capping it right now. That can be changed if needed.

  • The recent history will contain up to 99 recent objects but the search result will only show what is specified by the max size variable. This way if the user decided to increase the max size again, the data is still there.

-- Update max size
2020-07-22_18-09

-- Updated search result
2020-07-22_18-12


  • Font will change colors if there is a change in the max size value without confirming or rejecting the change. Helps track where the max size is set to.
    2020-07-22_18-25

  • Also, I was able to execute the code from my first question earlier. It turned out as expected.. for "alpha" only. Most (if not all others) seems to be sorted backwards.

-- Alpha (good)
alpha01
alpha04

-- Beta (bad) This is happening on the master branch too so I don't think I did anything to effect that.
image

@lgtm-com
Copy link

lgtm-com bot commented Jul 23, 2020

This pull request introduces 1 alert when merging a1bdeed into 14112d6 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@chithihuynh chithihuynh changed the title WIP: added recent search added recent search Jul 23, 2020
@lgtm-com
Copy link

lgtm-com bot commented Jul 28, 2020

This pull request introduces 1 alert when merging 74de9b8 into 50bf1e7 - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@chithihuynh
Copy link
Contributor Author

Hi @alex-w

I updated the code to include an option for max history size. But one of the tests failed because of a "redundant line" so I removed it. Then the updated code failed because of the "Travis CI" build errored out.

I am not sure what I did wrong or need to fix.

@alex-w
Copy link
Member

alex-w commented Aug 6, 2020

@Astrotourist-info in vacation now, but I had some discussion to him by voice. :) He want to see a dropdown list of previously searched objects before and without new search. @gzotti and @chithihuynh maybe just add a new additional tab with table (with few columns, e.g. name of object, him designation, type...) of previously searched objects?

@gzotti
Copy link
Member

gzotti commented Aug 6, 2020

Sorry, not before next weekend.

@chithihuynh
Copy link
Contributor Author

chithihuynh commented Aug 11, 2020

He want to see a dropdown list of previously searched objects before and without new search.

Sorry for taking so long. After trying a few different configuration for the drop down search, below is a demo of the drop down search. (It's only searching through a limited database). I have to incorporate the change into the real search dialog.

  • FYI: recent searches are now boded & currently selected objects are highlighted

1) Opened up demo search dialog (with recent already populated)
searchDropDownMenu01

2) Searching for "p"
searchDropDownMenu02

3) Scroll down
searchDropDownMenu03

4) Backspace until search is empty (recent objects will re-populate)
searchDropDownMenu04

maybe just add a new additional tab with table (with few columns, e.g. name of object, him designation, type...) of previously searched objects?

I can work on this after I finish the previous one. (I am going back to school next week so I can not commit to this anymore. Sorry)

@lgtm-com
Copy link

lgtm-com bot commented Aug 14, 2020

This pull request introduces 1 alert when merging 4957e70 into 505beed - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@chithihuynh
Copy link
Contributor Author

Here's the update version:
searchDropDownMenuA_01
searchDropDownMenuA_02

Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

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

No need tab "Recent" in this case.

@alex-w
Copy link
Member

alex-w commented Aug 16, 2020

Please enable opening first tab by default also

@alex-w
Copy link
Member

alex-w commented Aug 16, 2020

Please fix crash when user clicked at item of list after search

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2020

This pull request introduces 1 alert when merging 02f9ed2 into 7e839fa - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

Copy link
Member

@alex-w alex-w left a comment

Choose a reason for hiding this comment

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

OK, the new feature looks good, thanks!

I have one question: you are using white color for text in list of recent names, but previously was used black color for text for similar block - are you tested visibility of text with different colors?

@gzotti your opinion for feature please

@chithihuynh
Copy link
Contributor Author

I just left it as the default color but I think I can change it.

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2020

This pull request introduces 1 alert when merging 508a59f into 7e839fa - view on LGTM.com

new alerts:

  • 1 for Comparison result is always the same

@alex-w
Copy link
Member

alex-w commented Aug 16, 2020

Please fix LGTM alerts

@alex-w
Copy link
Member

alex-w commented Aug 16, 2020

Please back visual style :)

@alex-w
Copy link
Member

alex-w commented Aug 16, 2020

Would you add into the Options tab a button for clear history?

@gzotti
Copy link
Member

gzotti commented Aug 16, 2020

Nice feature!
I tried a few targets. Then I tried "Copernicus". At first, the star with exoplanets came up (rho1 Cnc). Then I switched on Planetary Nomenclature in the view settings/SSO tab. Searching for "Copernicus" again found "Copernicus (crater)" and "Copernicus (Krater)" (I am using German program language). However, both entries point to crater Copernicus on Mars. I then zoomed onto the Moon until I had the Copernicus label visible on screen. Searching for Copernicus still brought me only back to Mars. I don't know exactly how the search for planetary nomenclature can mess this up (does it provide only the first entry, and "Mars" is alphabetized before "Moon"? ) Searching for Hevelius was fine, so I assume my observation has to be solved outside this PR.

@gzotti
Copy link
Member

gzotti commented Aug 16, 2020

@chithihuynh Yes, additions to the User Guide with a detailed description are welcome. Before the merge! Please mark it as \newFeature{v0.20.3}

I have meanwhile found a solution for the problem mentioned above. It is indeed outside this PR, no need to bother.

@alex-w alex-w added this to the 0.20.3 milestone Aug 16, 2020
@chithihuynh

This comment has been minimized.

@alex-w

This comment has been minimized.

@chithihuynh

This comment has been minimized.

@alex-w
Copy link
Member

alex-w commented Aug 16, 2020

Please add description of new feature to the User Guide as final streak of PR

@chithihuynh
Copy link
Contributor Author

Sorry it took me a while for the user guide but I came across a git issue.

I accidentally commit a practice version then restore the "ch_interface.tex" file. So now, all the
"git blame" lines for that file points to me on the day I restored it.

Is there a way I can fix it? All my work was done on a new branch which has the old blames but when ever I try to put it on this branch, all the blame goes back to me.

@gzotti
Copy link
Member

gzotti commented Aug 17, 2020

I am not a git expert but I think you can totally remove a commit from your private git so a bad commit really vanishes (?)
So you could delete all traces and add your changes again. Of course, nobody else should have checked out your changes!
Google for detailed instructions.

@chithihuynh
Copy link
Contributor Author

chithihuynh commented Aug 18, 2020

Ok, I believe I fixed the problem regarding the user's guide git issue.

In the user's guide, in addition to adding the recent search information:

  • I also added subsections for each tab because I wanted to reference the Options tab a few times.
  • Added a new image of the Object tab that shows how the recent search results should look like.
  • Change typos that I noticed.
  • QUESTION: What is the "Extend search" button (mentioned in the Options tab)? I haven't been able to find it in the Search window.

@alex-w
Copy link
Member

alex-w commented Aug 18, 2020

  • QUESTION: What is the "Extend search" button (mentioned in the Options tab)? I haven't been able to find it in the Search window.

Where is you find it?

@chithihuynh
Copy link
Contributor Author

chithihuynh commented Aug 18, 2020

I found it in here

When the name of an object to find is typed in the object

I meant this line:

window and you are connected to the internet and ``Extend search'' is

@gzotti
Copy link
Member

gzotti commented Aug 18, 2020

Oops, seems we have updated the GUI but not the Guide. Please edit in the Guide and replace "Extend Search" by "Use SIMBAD".

@alex-w alex-w merged commit 30bf844 into Stellarium:master Aug 19, 2020
@chithihuynh chithihuynh deleted the recentSearchOneCommit branch August 25, 2020 16:57
@axd1967
Copy link
Contributor

axd1967 commented Oct 11, 2020

@chithihuynh this is a very nice feature, thank you.

do you think following would be possible: to delete an item from the recent searches by hitting the Delete button while an item is selected?

So in this case, I use cursor control to descend to "SOYUZ..." and then hit the Delete button to remove that item from the list.

image

@chithihuynh
Copy link
Contributor Author

@axd1967
Thank you. Sorry about the late reply. I am currently in school so I didn't see this until now.

I think that would be a great feature to add in. If you put in an issue request, I can get to it this summer (sorry I can't get to it sooner).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Entirely new feature
Development

Successfully merging this pull request may close these issues.

Remember last entered objects
4 participants