Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#955 closed defect/bug (fixed)

List of former destinations for internal GUI: Destinations should not appear multiple times

Reported by: sleske Owned by: KaZeR
Priority: minor Milestone:
Component: core Version: git master
Severity: Keywords:
Cc:

Description

At the moment, every time a destination is set, it gets added to the list of "former destinations" (as visible in the internal GUI, and in destination.txt). This happens even if the destination was selected from the list of former destinations, thus creating a duplicate entry in the list.

This does not make sense. If a destination is set that is already in the list of "former destinations", its existing entry should just be moved to the top of the list, not duplicated.

Attachments (3)

no-duplicate-dests.diff (11.5 KB) - added by sleske 9 years ago.
Patch updated. assert.h removed, g_strcmp0 instead of strcmp. Applies to r4846.
g_list_free_full.diff (1.2 KB) - added by sleske 9 years ago.
Add g_list_free_full to the support libs (copied from original glist source).
no-duplicate-dests-no-fancy-glib.diff (11.6 KB) - added by sleske 9 years ago.
New version of my patch (really this time). No more fancy glib functions: g_strmcp0 & g_list_free_full taken out. former_destination->description is now allocated dynamically.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by sleske

I am working on fixing this. I'll try to submit a patch in a few days.

comment:2 Changed 9 years ago by sleske

The attached patch fixes the issue. The fix consists mostly of changes to bookmarks_append_coord (bookmarks.c), which I had to rewrite almost completely. The function now reads the list of former destinations into a list in memory, then uses that list to find and discard duplicate entries.

As it has really nothing to do with bookmarks, I also renamed bookmarks_append_coord to formerdests_append_coord. Of course, if the old name is desired due to naming conventions, feel free to change it back.

The patch also contains a few minor fixes I found helpful while developing, mostly removal of unused function params.

comment:3 Changed 9 years ago by sleske

Effect of the patch:

The patch prevents duplicate entries in the list of former destinations. When a destination is selected that is already in the list of former destinations, it is just moved to the top of the list, rather than being added another time.

Note: Two destinations are only considered the same if both coordinates and description text (and type) are the same. So if you add the same destination with two different names (e.g. by using two bookmarks with identical coords but differen names), both will appear in the list.

comment:4 Changed 9 years ago by sleske

Final note:

The patch fixes the issue for both the internal GUI and the GTK gui.

The fix only changes parts that are GUI-independent, so it should work for the other GUIs as well (however I did not test that).

comment:5 Changed 9 years ago by tegzed

Hi Sleske,

Thanks for your good work, I haven't checked the code but I have tried the patch and it works OK for me.

comment:6 Changed 9 years ago by woglinde

Hi,

some nitpicking, Please remove the unused assert.h and replace all string compare functions with the glib ones.

Bye Henning

Changed 9 years ago by sleske

Patch updated. assert.h removed, g_strcmp0 instead of strcmp. Applies to r4846.

comment:7 Changed 9 years ago by tryagain

I think I know much simplier way to do this. No need for temporary lists at all.

Bookmarks_append_coord will look like:

1. Write new entry into output text file.
2. Let counter=limit-1
3. loop for each element in the bookmarks map while counter-->0
3.1. If current element equals to new element or has wrong type, continue
3.2. Write current element into output file
Last edited 9 years ago by tryagain (previous) (diff)

comment:8 Changed 9 years ago by sleske

Hm, I'm don't think that is practical.

The two main problems:

  • Since the latest entries come last in destination.txt, we need to truncate the beginning of the file if there is a limit. Thus we always need two passes through the file (one to find the current number of entries, then one more to move the newer entries to the beginning of the file). I don't see how you could do that in one pass, because you only know which entries you need to keep after reading the whole file (since you need to keep the *last* n entries, and there's no (easy) way to read a file backwards).
  • I wanted to avoid duplicating the parsing code in map/textfile.c, so I used the textfile map methods to read destination.txt. However, the textfile code does lazy loading, so I can only start changing destination.txt once everything has been read -> again, not possible without some temp storage.

So I don't see how your idea will work.

Of course, feel free to submit a patch :-).

comment:9 Changed 9 years ago by tryagain

Well, you're right. With current textfile code my solution (if we fix it to change items order) will read the file twice, which is impractical.

Bad news is that we currently have no g_list_free_full in support/glib.c, so your patch will break building navit on platforms without system-supplied glib.

So we need to re-implement (or steal from current glib) g_list_free_full.

comment:10 Changed 9 years ago by sleske

Oops, I did not know that. I thought I could use everything that's in glib, sorry.

But I hope g_list_free_full does not pose a problem. I've looked at the glib sources, and it's only two lines:

g_list_foreach (list, (GFunc) free_func, NULL); 
g_list_free (list);     

Is it enough if I just copy the necessary code from glib to navit/support/glib/glist.c ? Then I'll just make a second patch.

Also, I have no idea how to test the stuff under navit/support. Can someone else do that?

Changed 9 years ago by sleske

Add g_list_free_full to the support libs (copied from original glist source).

comment:11 Changed 9 years ago by tryagain

Bad news again:

  1. It doesn't build for android, fails on missing g_strcmp0.
  2. It doesn't build on Ubuntu 10.04 LTS, fails on missing g_list_free_full. We can't use that function from support/glib because going that way we will miss too much linux specific bonuses like gtk, dbus...

After these two issues fixed (both trivial, so no patches) it builds fine for me, and version under linux even runs. No time to run on android.

And I'll repeat my suggestion from http://irclogs.navit.ie/%23navit-2011-11-10.log (look after 21:35):

In "struct former_destination" i'd better had member description allocated dynamically. For any given fixed buffer size there may be a string which will be truncated. For Cyrillic, you're limiting description length by about 25 characters, last of which might be broken due to wrong (non-utf-8-aware) truncation.

comment:12 Changed 9 years ago by sleske

@tryagain: Sorry, didn't see your comment on IRC, I don't always scan the logs. If you have comments about a patch, better put them into trac, there I'll see them.

Anyway, point taken, I forgot about the UTF-8 issue. My new patch above fixes this.

The new patch replaces the two older patches. It contains the changes from the first patch; the second patch (g_list_free_full.diff) is no longer needed, as the new patch no longer uses g_list_free_full.

comment:13 Changed 9 years ago by tryagain

You seem to have uploaded wrong file. Both "char description[51]" and "g_free_full(" are still there. Please try again ;)

Changed 9 years ago by sleske

New version of my patch (really this time). No more fancy glib functions: g_strmcp0 & g_list_free_full taken out. former_destination->description is now allocated dynamically.

comment:14 Changed 9 years ago by tryagain

  • Component changed from core to gui/internal
  • Resolution set to fixed
  • Status changed from new to closed

Patch no-duplicate-dests-no-fancy-glib.diff by sleske is applied in http://navit.svn.sourceforge.net/viewvc/navit?view=revision&revision=4847

comment:15 Changed 9 years ago by sleske

  • Component changed from gui/internal to core

Thanks for applying :-).

Nitpick: I changed component from gui/internal back to core. This bug manifests itself in destination.txt, which is used by at least the internal and the GTK gui. It's not specific to the internal GUI.

Note: See TracTickets for help on using tickets.